-
Notifications
You must be signed in to change notification settings - Fork 510
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
chore(icons): replace badges.svg with icon classes #5644
Conversation
946b17e
to
83831dd
Compare
The test needed to use the selector |
Side Note: I got side tracked. I manually looked at every result from https://github.com/mdn/content/search?q=obsolete. https://developer.mozilla.org/en-US/docs/Web/API/RTCDataChannel#obsolete_properties and https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection#obsolete_events are the only pages out of those still using the obsolete icon (rather than just the word obsolete). |
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 will have to check internally if there is consensus for removing the color of those icons, but the changes themselves make sense. Thanks!
Misclicked and ended up requesting a review closing and reopening. |
I added the colors back until a decision is reached, which should make this mergeable before then. To remove color from the sidebar, these lines should be removed: yari/client/src/document/organisms/sidebar/index.scss Lines 179 to 187 in 6e13bf8
|
34e7d0a
to
6e13bf8
Compare
Added a commit to fix #5844. |
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.
LGTM, but I will request a 2nd review from a peer
@schalkneethling This looks good to me, but maybe we can even remove the badge.svg, since it's no longer referenced anywhere (as far as VSCode search tells)? |
I thought the same, but was just worried I may have missed something. I can remove it if we're sure it's now un-used. |
Hey @danielhjacobs and @caugner. I will take a look at this one today. Thanks for the ping. |
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.
Apologies for taking so long to get back to you. This looks good to me. Thanks @danielhjacobs 🎉
Summary
Fixes the remainder of number 3 (deprecated icon is different in badges.svg) from #5571 and just seems nicer
Also fix #5844
Problem
Stop using badges.svg.
Solution
Uses the same type of code used for the BCD
Screenshots
Before
After