Skip to content
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

Open
wants to merge 9 commits into
base: gh-pages
Choose a base branch
from
Open

Add dark mode support #130

wants to merge 9 commits into from

Conversation

xfq
Copy link
Member

@xfq xfq commented Mar 14, 2024

Fix #129.


Toggle dark mode in your system to see the effect:


Preview | Diff

Copy link

netlify bot commented Mar 14, 2024

Deploy Preview for bp-i18n-specdev ready!

Name Link
🔨 Latest commit ecc58e4
🔍 Latest deploy log https://app.netlify.com/sites/bp-i18n-specdev/deploys/663858bb7ccd4c00086ab501
😎 Deploy Preview https://deploy-preview-130--bp-i18n-specdev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@aphillips aphillips left a 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,
Copy link
Contributor

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)

Copy link
Member Author

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 .

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member Author

@xfq xfq Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most style guides now recommend using spaces, like:

That said, I think large-scale modifications to whitespaces also have costs and consistency within a file is more important.

@xfq xfq marked this pull request as draft March 20, 2024 06:38
@xfq
Copy link
Member Author

xfq commented Mar 20, 2024

Do we have a plan to port to all specs?

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.

@xfq xfq marked this pull request as ready for review March 22, 2024 06:57
@xfq
Copy link
Member Author

xfq commented Mar 22, 2024

I updated some colours and removed some styles that were no longer useful.

Ready for review/merge now.

@xfq
Copy link
Member Author

xfq commented Mar 22, 2024

@marcoscaceres
Copy link
Member

I think local.css needs to be updated to support @media (prefers-color-scheme: dark) for those <aside class="links">. That's not under ReSpec's control.

--summary-text: rgb(236 164 113);
--uname-text: rgb(215 99 99);
--xref-bg: rgb(53 42 11);
}
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dark mode
4 participants