-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Rating] Add a demo with different icons #19004
Conversation
No bundle size changes comparing 25d7040...08811eb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, it's a great start!
A docs demo would be good, for example using the sentiment icons.
Thanks! I'll work on this soon. Any CSS ideas on how to align these icon to baseline? |
@hoop71 Thanks for exploring this issue. Regarding the implementation, I don't think that we need to significantly change the source nor the API. I believe a new demo would require most of the work (leveraging the existing |
Got it. This makes a lot of senes. |
OK - I think this is working pretty well with the Cheers. |
For the sentiment example it would be good to provide custom text labels using Not sure what's going on with the size example. (Do you have a practical use-case for this?) If you edit the typescript demo, the JS demo can be generated with |
Yes, I can do this in typescript and getLabelText, no problem. The example came from the orginal ask in #18971. I guess it depends on how much you'd like to show in the demo or let users figure out? |
Co-Authored-By: Olivier Tassinari <olivier.tassinari@gmail.com>
Regarding the size issue, all the rating needs to have the same width for the logic to work correctly. It's a simplification of the logic. We are lucky it's probably a good constraint for UX. |
In the sample image in the issue those look like different icons, rather than the same icon resized (they're all the same width). I think we can safely drop the size demo as @oliviertassinari suggested. |
c02231b
to
33fd6cc
Compare
d9c073d
to
08811eb
Compare
@hoop71 Thanks |
Starts to provide a solution for #18971. Thinking I will round this out and also add a prop that allows the user to align the icons as desired.
Would love feedback from the team if this seems like a good path? @oliviertassinari
Closes #18971.