-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Remove miscellaneous css custom properties #4620
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
Conversation
size-limit report
|
alex-page
left a comment
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.
This looks great @lgriffee. I am unsure if we can remove some of these values all together or if they override another value in the CSS cascade. Would you be able to take a look?
| .placeholder { | ||
| // stylelint-disable-next-line selector-max-class | ||
| &.error .Input { | ||
| // This is the only place this color is used. | ||
| // stylelint-disable-next-line color-no-hex | ||
| color: #9c9798; | ||
| } | ||
|
|
||
| // stylelint-disable-next-line selector-max-class, selector-max-specificity | ||
| &.error .Input:-moz-focusring { | ||
| color: transparent; | ||
| text-shadow: var(--p-override-none); | ||
| } | ||
| } |
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.
Removing unused class. Here's the legacy commit from when it was added
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.
Amazing! Thanks for the context Laura. I think it makes sense to remove this code.
|
|
||
| .ContentWrapper { | ||
| background: var(--p-override-transparent); | ||
| } |
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.
Do we need the HTML element with .ContentWrapper classname? Or can we flatten this components HTML?
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 don't think we need it—I can consolidate its relevant CSS into .Breadcrumb and replace its HTML with a shorthand fragment so that contentMarkup still has one parent element (which you can check out in this commit).
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.
Also along the line of consolidation I just want to note that there might be more rollback values that need to be removed in these files. Not sure if there was work done already to remove them so it’s definitely possible that they are needed overrides. I didn’t look into them in this PR to keep a narrow scope. But if it’s something important to address now or in the future I can create a ticket to look into it.
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.
Awesome work @lgriffee. I had one question but this looks great 💯. It looks like you have a conflict with the UNRELEASED.md file let me know if you need help resolving that!
aveline
left a comment
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.
This looks great @lgriffee!
kyledurand
left a comment
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.
Looks good just a few comments. I'll 🎩 now
Co-authored-by: Kyle Durand <kyledurand@users.noreply.github.com>
Co-Authored-By: Kyle Durand <6844391+kyledurand@users.noreply.github.com>
kyledurand
left a comment
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.
Sorry about the delay 🎩 ing. I ran into a nix issue with dev last night and got distracted with other things. This looks great! Excellent clean up. The only thing I noticed via chromatic is that the breadcrumb back button is slightly wider but I think it's an improvement now that it's perfectly square 👍
I left a note about removing one more sass var that appears to be unused, and unreleased has some entries that can snuck in by a rebase probable but otherwise 🚢 !
Co-Authored-By: Kyle Durand <6844391+kyledurand@users.noreply.github.com>
WHY are these changes introduced?
Fixes #4589
The following tokens (see table below) were made to rollout the design language changes in 2020 and should no longer be used.
WHAT is this pull request doing?
Swapping the following tokens for equivalent values or completely removing the value and associated css property.
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx:🎩 checklist
README.mdwith documentation changes