-
Notifications
You must be signed in to change notification settings - Fork 67
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
feat(icons): updated star icons, added star ratings #1585
Conversation
75da465
to
cb528a2
Compare
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.
I think we'll need to come up with a better approach that leverages pure CSS. Dark mode can only trigger a CSS media query, it can't modify HTML.
How about if the star-half
SVG contains two paths (left side and right side), and each path has a class that can be targeted in the media query? Then you can modify its fill/color. You might not need even need class, maybe just nth child selector will do.
If this works we can then delete star-half-dark
.
96a774d
to
ea590d8
Compare
ea590d8
to
566d8eb
Compare
This seems to work pretty well for both color modes:
The trick is Might just need a little fine tuning to get the right shade/opacity. @austingardner You could apply the same technique to the star ratings and then go ahead and delete all the "dark" versions. p.s. I also added an update to Gulp which now auto syncs the SVG, without needing to stop the server and rebuild. |
566d8eb
to
6b0f76c
Compare
One general comment is that we might want to start looking into breaking up this large icon file by type, e.g. star series, shipping, navigation, etc. The file is getting pretty huge now (~123kb). We could do this in a backwards compatible way where We'd need a simple package script that would stitch all the individual files together into the large file. |
I'd be interested in taking that project on! |
6b0f76c
to
caf5091
Compare
caf5091
to
1dfefec
Compare
So the more I look at it, I'm not sure that star ratings belong in the icon file. They don't feel like icons. They feel like a different thing altogether. I'm going to either move or comment them out of this PR temporarily and then we can revisit in #1599 |
We can probably do something similar like we did with program-badge and have a star-rating file. |
Yep, I think that's what I'll go with. |
So I separated out the star-rating from icon into its own module. This addresses the non-interactive part of #1599. The interactive star rating can be tackled in a future PR. Some other cleanup:
|
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.
Love the new changes! They look great and much cleaner! Bravo!
Description
Updated individual star icons, as well as added the "star ratings" icons - which are rows of stars that allow ratings to be quickly displayed.
Context
One issue I ran into was that certain icons with two colors can't be filled by default, and I'm not sure how I would change their colors with CSS. So I ended up having a "light" mode and a "dark" mode for
star-half
as well as the "star ratings" icons. Maybe we can come up with a better solution in the future.References
closes #1473
Screenshots