Add core component changes for modern v5.x colors #4915
Conversation
|
I thought about if these changes should live in I could instead set the variables I need to set in |
Fix compile issuesPreviews: documentation | landing | table |
Add testing app to test component changesPreviews: documentation | landing | table |
| } | ||
|
|
||
| // Contrast for icons | ||
| .#{$ns}-icon:not([class*="#{$ns}-intent-"]) { |
There was a problem hiding this comment.
By default the non-intent colors use $pt-text-color instead of $pt-icon-color (i.e. $pt-text-color-muted) and likewise for dark mode, because they were being imported from body:
I've overwritten the color here so that we don't just default to the default text colors/to match the colors that are in the proposed designs, but I'm not sure if this will break anything down the line. I.e., if we're breaking icon color inheritance somewhere else
There was a problem hiding this comment.
Icons are supposed to inherit text color in this way, so yeah, I think this could break something further up the application stack.
What was the goal of setting this particular line with color? Where do we see it in effect?
There was a problem hiding this comment.
This was originally to fix the info icon for callouts, dialogs, and toasts. I was able to fix these in each of those components so this is no longer necessary
Tweak examples and fix slider stylesPreviews: documentation | landing | table | demo |
Fix checkbox/radio stylesPreviews: documentation | landing | table | demo |
Add disabled component examples and fix switch stylesPreviews: documentation | landing | table | demo |
Fix disabled button stylesPreviews: documentation | landing | table | demo |
Fix icon colors for toastPreviews: documentation | landing | table | demo |
Fix tooltip examplePreviews: documentation | landing | table | modern colors demo |
Remove unnecessary dependencyPreviews: documentation | landing | table | modern colors demo |
| } | ||
|
|
||
| // Contrast for icons | ||
| .#{$ns}-icon:not([class*="#{$ns}-intent-"]) { |
There was a problem hiding this comment.
Icons are supposed to inherit text color in this way, so yeah, I think this could break something further up the application stack.
What was the goal of setting this particular line with color? Where do we see it in effect?
|
Some dots on the last demo link above:
|
Fix for commentsPreviews: documentation | landing | table | modern colors demo |
|
@aycai addressed your design comments. Only thing that I didn't directly touch was your comment about the HTML table - the blue text I used there is just hard coded, since we don't have any default intent colors for |
|
Ahh shoot I misspoke re: html table dark mode - I was looking at the background color for the hover state that looked closer to the previous gray in hue. Sorry about that! |
Address design commentsPreviews: documentation | landing | table | modern colors demo |
|
Final nit on tree: not sure how icon color is defined in that context but it should be $color5 for dark mode, looks like it's $color2 (which is the color for light mode) in the test app. Otherwise updates look good! |
|
@aycai fixed! |
Fix dark mode icons for treesPreviews: documentation | landing | table | modern colors demo |
Changes proposed in this pull request:
Not done in this PR (punting for future PRs)
packages/core/src/blueprint-modern.scss- break Sass code out into individual files based on componentReviewers should focus on:
Screenshot