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.

Sign up for GitHub

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

Jump to bottom

feat: support dark mode #612

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

feat: support dark mode #612

wants to merge 5 commits into from

Conversation

kirklin
Copy link
Contributor

@kirklin kirklin commented Mar 19, 2024 •
edited by pr-preview bot

Fixes #611

Preview | Diff

@netlify Netlify
Copy link

netlify bot commented Mar 19, 2024 •
edited

✅ Deploy Preview for clreq ready!

Name Link
🔨 Latest commit 248bff9
🔍 Latest deploy log https://app.netlify.com/sites/clreq/deploys/66385a93a09ac500082a5bec
😎 Deploy Preview https://deploy-preview-612--clreq.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.

@xfq
Copy link
Member

xfq commented Mar 20, 2024 •
edited

Thanks for your PR, @kirklin!

Unfortunately, simply adding darkMode: true is not enough. We need to customize the colors in CSS with @media (prefers-color-scheme: dark) {...} and CSS custom properties, like

clreq/local.css

Lines 147 to 160 in 60649c5

background-color: #fbc481;
}
#langSwitch button:lang(zh-hant) {
background-color: #9fe19f;
}
.translateme, .retranslateme {
background-color: #F3F3C3;
background-clip: content-box;
}
.checkme {
background-color: #BBEFFC;
and

clreq/local.css

Line 59 in 60649c5

color: #005a9c;

@xfq
Copy link
Member

xfq commented Mar 20, 2024

We also need to make dark mode versions for at least some of the images in the document.

xfq
xfq reviewed Mar 25, 2024
View reviewed changes
index.html Outdated Show resolved Hide resolved
xfq
xfq reviewed Mar 25, 2024
View reviewed changes
dark.css Outdated
Comment on lines 17 to 23
#langSwitch button:lang(zh-hans) {
background-color: #005a9c;
}

#langSwitch button:lang(zh-hant) {
background-color: #0171c4;
}
Copy link
Member

Choose a reason for hiding this comment

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

These two colours are too close to each other. The different colours here are to allow people who do not understand Chinese to better distinguish Traditional and Simplified Chinese.

In addition, we need to update

clreq/local.css

Lines 183 to 188 in 7d51569

.is-multilingual [its-locale-filter-list="zh-hant"] {
border-color: rgba(64,196,64,.5);
}
.is-multilingual [its-locale-filter-list="zh-hans"] {
border-color: rgba(248,138,5,.5);
}
to make them consistent.

xfq
xfq reviewed Mar 25, 2024
View reviewed changes
dark.css Outdated
}

.checkme {
background-color: #627262;
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I would prefer a darker background, but not strong enough to object this.

xfq
xfq reviewed Mar 25, 2024
View reviewed changes
index.html
@@ -366,7 +367,7 @@ <h5>
</h5>

<figure id="fig-loose-setting">
<span style="text-align:center;"><img src="images/en/increased-inter-character-spacing.svg" alt="Examples of loose setting in horizontal writing mode." width="600" height="209"></span>
<span style="text-align:center;"><img class="invert-on-dark-mode" src="images/en/increased-inter-character-spacing.svg" alt="Examples of loose setting in horizontal writing mode." width="600" height="209"></span>
Copy link
Member

Choose a reason for hiding this comment

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

Our theme colour, green, will be missing if we invert the images. We'll discuss if we need to fix this in the teleconference.

@xfq
Copy link
Member

xfq commented Mar 25, 2024 •
edited

Thanks for updating the PR! I made some comments.

IMHO we can consider using CSS custom properties (like https://github.com/w3c/bp-i18n-specdev/pull/130/files ), so that when updating the colour, it is easier to remember that we also have a dark theme that needs to be updated.

@xfq
Copy link
Member

xfq commented Mar 25, 2024

And we also need to update

clreq/local.css

Line 327 in 7d51569

border-bottom: 1px solid rgba(0, 0, 0, .42);

@kirklin kirklin requested a review from xfq March 26, 2024 06:33
@kirklin
Copy link
Contributor Author

kirklin commented Mar 26, 2024 •
edited

image
I'm wondering if we should just reverse this colour. make the green for Traditional Chinese directly into purple for Traditional Chinese.

@xfq
Copy link
Member

xfq commented May 6, 2024

Per w3c/respec#4700 , please remove darkMode: true and use:

<meta name="color-scheme" content="light dark">

@xfq
Copy link
Member

xfq commented May 19, 2024

There are still some problems with manually switching the colour theme. Let’s wait until w3c/respec#4687 is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xfq xfq Awaiting requested review from xfq

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dark mode
2 participants
@kirklin @xfq