-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Addon-docs: Fix table dark mode #14251
Addon-docs: Fix table dark mode #14251
Conversation
@shilman I think this could be tested using Chromatic, but not sure how to proceed, I'm guessing it'll run automatically as part of CI, the issue here is that the official-storybook example is using light mode for the docs, should I replicate that example with a new one using dark-mode or what would be the way to go? |
@domyen @ghengeveld can one of you please review? |
@shilman @ndelangen in this case I didn't add unit tests as I think visual regression is a better fit for these changes. These docs that include the MDX table are already in a story but it's using light mode in that example project, should I add anything extra for this or is it ok the way it is? (Also I'm not sure if this example project is getting tested in Chromatic) |
Oh I just saw the story in the chromatic link in CI 😄 https://www.chromatic.com/test?appId=5a375b97f4b14f0020b0cda3&id=60517a9dfa63c80021059690, but there are no tests for dark mode, should those be added? |
I don't see it as required to merge this PR, but if we could add a story like https://www.chromatic.com/test?appId=5a375b97f4b14f0020b0cda3&id=60517a9dfa63c80021059690 but in dark mode, that would be |
feb1343
to
2322e1c
Compare
@ndelangen Added dark mode story! 😄 so this is ready for merging I think, the only issue is that
|
2322e1c
to
5cdb429
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.
Looking good!!! 🙏
Issue: #14029
What I did
light
theme before.color.darkest
as the value forappContentBg
in the dark theme, as it was using the same value.This is how the tables look now in Dark mode:
And in Light mode there were no visual changes:
How to test
Manual Test
theme.light
totheme.dark
:storybook/examples/official-storybook/preview.js
Line 161 in df27d3a
cd examples/official-storybook
andyarn storybook
. (Also runyarn build core components addon-docs theming
in the root folder to update the build with the theme changes).Questions
For setting the alternative rows background color (pair rows in this case), I did the following:
I'm not sure if this was the way to go, I also thought of adding a new theme variable, something like
theme.appAlternativeContentBg
, but not sure if this would be used that often.