Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
T345056: dark mode #186
T345056: dark mode #186
Changes from all commits
23fdab3
d230e53
af409ca
89e7ceb
dbd6a3d
679c04a
51648b8
21f0c48
06edf0f
9cefc10
34179de
4ddeeff
155d767
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 55 in src/stories/preview.stories.js
GitHub Actions / build (17)
Check warning on line 55 in src/stories/preview.stories.js
GitHub Actions / build (latest)
Check warning on line 56 in src/stories/preview.stories.js
GitHub Actions / build (17)
Check warning on line 56 in src/stories/preview.stories.js
GitHub Actions / build (latest)
Check warning on line 59 in src/stories/preview.stories.js
GitHub Actions / build (17)
Check warning on line 59 in src/stories/preview.stories.js
GitHub Actions / build (latest)
Check warning on line 63 in src/stories/preview.stories.js
GitHub Actions / build (17)
Check warning on line 63 in src/stories/preview.stories.js
GitHub Actions / build (latest)
Check warning on line 65 in src/stories/preview.stories.js
GitHub Actions / build (17)
Check warning on line 65 in src/stories/preview.stories.js
GitHub Actions / build (latest)
Check warning on line 93 in src/stories/preview.stories.js
GitHub Actions / build (17)
Check warning on line 93 in src/stories/preview.stories.js
GitHub Actions / build (latest)
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.
Some of the duplication with less mixins could be avoided with CSS custom properties (are they okay to use in this project?). Example:
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 see why it wouldn't be okay to use them, I refactored to use CSS custom properties and indeed it seems a bit cleaner (this way seems to be more of a standard - https://web.dev/articles/prefers-color-scheme#stylesheet_architecture), thanks
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.
:root
represent the whole document, probably the<html>
or<body>
tag. It's important to remember that this is a small component hosted within any website. If we want to use an approach like that, everything has to be prefixed so that even if it bleeds into the host, it has no side effects on 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.
LESS variables would probably give you a similar coding style but with actual isolation.
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.
Oh that's a good point, I can update using LESS variables and maybe prefix it anyway as something like
--wikipediapreview-primary-background-color
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.
Leaving it with prefixes for now, let's make a decision next week
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 good now i think, but can we still use mixin() here so we don't repeat the --var settings?
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 to repeat the styles for the "light" variants? Aren't those the defaults?
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.
can we also write in README to explain how the site owner custom their dark mode styling?
custom code like this, and explain what's the var there
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.
Great feedback all, thanks