-
Notifications
You must be signed in to change notification settings - Fork 31
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: Theming Iris Grid #1568
feat: Theming Iris Grid #1568
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1568 +/- ##
==========================================
+ Coverage 46.45% 46.47% +0.01%
==========================================
Files 571 572 +1
Lines 36030 36060 +30
Branches 9020 9026 +6
==========================================
+ Hits 16738 16759 +21
- Misses 19240 19249 +9
Partials 52 52
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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.
Not sure why some of the grid tests are failing - looks like the colour isn't matching up. Double check that the correct colour is being used, if it is a new colour then update the snapshots (though I'm confused why can open a simple table
passes but rollup/goto don't).
@@ -12,92 +12,90 @@ $header-separator-color: $gray-900; | |||
$header-height: 30px; | |||
|
|||
:export { | |||
grid-bg: $gray-900; | |||
grid-bg: var(--dh-color-grid-bg); |
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.
Many of the variables above are no longer used (e.g. $selection-color
).
Also the new selection color doesn't match the old color; I think that may be intentional? Confirm with @dsmmcken , but that's one reason the goto row tests are failing.
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.
--dh-color-grid-tree-line: var(--dh-color-gray-700); | ||
--dh-color-grid-tree-marker: var(--dh-color-gray-800); | ||
--dh-color-grid-tree-marker-hover: var(--dh-color-gray-900); |
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.
These don't match what they were before:
tree-line-color: $gray-500;
tree-marker-color: $gray-300;
tree-marker-hover-color: $gray-100;
If that's intentional, then need to update snapshots. Seems odd we're going in the reverse on the gray scale though (the scale may not be the same anymore? Dunno).
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.
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've updated the tree variables to be closer to the original for now. Seems to have fixed these tests
14df256
to
4e2d70e
Compare
Testing
Current mapping of Bootstrap gray palette to new
--dh-color-gray
vars based on Don's POC. The Bootstrap variables will get updated in a future PR to pull from the new vars, but this is how things line up right now:BREAKING CHANGE: Enterprise will need ThemeProvider for the css variables to be available