Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding dynamic $size to filament badge component #12784

Open
wants to merge 15 commits into
base: 3.x
Choose a base branch
from

Conversation

tnylea
Copy link
Contributor

@tnylea tnylea commented May 14, 2024

In this PR I've added the badgeSize prop to the Icon Button component. This will allow the user the ability to modify the size of the badge by passing it through as a prop to the parent Icon Button component 👍

Let me know if you have any questions about this PR. Appreciate it.

@tnylea
Copy link
Contributor Author

tnylea commented May 14, 2024

I've also added this prop to the default button component as well. Instead of hardcoding the badge as xs, this will allow the user to change the size of the badge from the button and icon-button component.

@danharrin danharrin added the enhancement New feature or request label May 14, 2024
@danharrin danharrin self-assigned this May 14, 2024
@danharrin danharrin added this to the v3 milestone May 14, 2024
Copy link
Member

@danharrin danharrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do the same for the link component :)

@zepfietje zepfietje assigned zepfietje and unassigned danharrin May 15, 2024
@zepfietje
Copy link
Member

Could you share the use case for this, @tnylea?
Also, please provide screenshots for all components with all badge sizes. 🙂
The current positioning has been fine-tuned to work with the current size, so we should make sure all cases are covered for any new combinations we add too.

@tnylea
Copy link
Contributor Author

tnylea commented May 17, 2024

Thanks for the feedback guys.

I'll be adding this to the link component as well. I see that I need to make some layout modifications based on the size.

When we increase the badge size to an XL, it looks like it sits too low on the element.

CleanShot 2024-05-17 at 16 37 17@2x

So, I'll work to get this to show correctly for all sizes. Just wanted to keep you posted that this is on my radar and I'll work on getting this updated on the weekend or upcoming week.

I'll be sure to include screenshots of all elements and all sizes of the badge.

Hope you guys are having a great Friday!

Talk soon!

@zepfietje
Copy link
Member

Yeah that's exactly what I meant.
Thanks, Tony!

@tnylea
Copy link
Contributor Author

tnylea commented May 20, 2024

Hey guys,

This PR should be good to go unless there is anything else you see on your end. I have made sure to merge the latest upstream/3.x with my branch as well.

Here is the screenshot of the original badge size from this PR:

CleanShot 2024-05-20 at 19 21 49@2x

This is for the icon-button, button, and link components. Here are the screenshots for sizes sm, md, and lg.

CleanShot 2024-05-20 at 19 22 12@2x
CleanShot 2024-05-20 at 19 22 42@2x
CleanShot 2024-05-20 at 19 23 09@2x

Let me know if these look good or if we need any other modifications :)

Appreciate it!

Copy link
Member

@zepfietje zepfietje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update and screenshots, @tnylea!

Has the horizontal displacement of the badge changed from the original position? I think it's a little too far away from the link specifically. Not sure though, so that's why I'm asking. Also I think it should be tailored for every badge size.

PS: Could you update the screenshots for the link, since they all use the sm badge size.

@tnylea
Copy link
Contributor Author

tnylea commented May 23, 2024

Thanks @zepfietje, I'll be circling back around to this today 😉

@tnylea
Copy link
Contributor Author

tnylea commented May 24, 2024

Important

I'm not sure why PHPStan tests are failing. They seem to be throwing an error on some of the files that I did not even change 🤷‍♂️ Any help on this would be appreciated 😊


Right you were @zepfietje, I needed to add some additional styles to the link badge in order to get it to look correct with all different sizes. I see that the options for badge size are xs, sm, and md. I guess if you pass lg or xl, it will use the same styles as md.

Here is how it looks right now.

CleanShot 2024-05-24 at 11 52 24@2x

CleanShot 2024-05-24 at 11 59 31@2x

CleanShot 2024-05-24 at 11 59 59@2x

The last one may look a little weird because I didn't add enough padding to the middle and bottom element, but you can see that the badges look good with all sizes.

I doubt that many people will use the md badge size on these elements because it does look a little large for that specific use-case, but I can definitely see users reaching for the sm badge size over the xs badge size, so it's good to have all these options.

Let me know if there's anything else you would like me to update and I can make those changes. Looking forward to getting this merged in 😉

Thanks! Talk to you soon.

@tnylea tnylea requested a review from zepfietje May 24, 2024 16:41
@tnylea
Copy link
Contributor Author

tnylea commented May 29, 2024

@zepfietje Tests seem to be passing now. Let me know if there's anything additional you want me to add to this one and I can make it happen.

Thanks! Look forward to hearing from you soon.

@zepfietje
Copy link
Member

Perfect, thanks Tony! Will be reviewing and merging this next. 👍

@tnylea
Copy link
Contributor Author

tnylea commented May 30, 2024

Great! Thanks @zepfietje 😊

@dissto
Copy link
Contributor

dissto commented May 30, 2024

Small change, big impact. Thank you. I have been needing proper spacing for my badges numerous times. I suppose now the "hacking" stops 👍💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants