-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add dark mode support #130
base: gh-pages
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for bp-i18n-specdev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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. Fix the formatting.
Do we have a plan to port to all specs?
index.html
Outdated
@@ -15,6 +15,7 @@ | |||
<script async src="https://www.w3.org/Tools/respec/respec-w3c" class="remove"></script> | |||
<script class="remove"> | |||
var respecConfig = { | |||
darkMode: true, |
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.
Use spaces not tabs (globally, e.g. in local.css below also)
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.
Thanks. I've updated index.html
.
Regarding local.css
, it seems like most of it uses tabs. I don't have a strong opinion (but tend to use spaces for personal projects), but if we decide to use spaces, maybe we should document it in https://www.w3.org/International/i18n-activity/guidelines/editing .
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.
Why convert tabs to spaces?
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.
Because the rest of the file is spaces. And because it makes it easier to maintain alignments. Whatever we do, we shouldn't mix the two indentation schemes.
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.
Yes. As mentioned in #129, if it's good enough, we can make our other documents (LTLI, counter styles, string-meta, string-search, timezone, lreq, and so on) support it too. This PR is not ready to merge yet, I still need to double check all the colors before merging. |
I updated some colours and removed some styles that were no longer useful. Ready for review/merge now. |
Filed https://github.com/w3c/respec/issues/4687 After merging this PR, I'll look into updating https://www.w3.org/International/i18n-activity/guidelines/editing#localcss |
I think local.css needs to be updated to support |
--summary-text: rgb(236 164 113); | ||
--uname-text: rgb(215 99 99); | ||
--xref-bg: rgb(53 42 11); | ||
} |
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.
Probably need to add .links {}
here too
Fix #129.
Toggle dark mode in your system to see the effect:
Preview | Diff