Add color_scheme_dark member and themeable members (colors only)#1207
Add color_scheme_dark member and themeable members (colors only)#1207christianliebel wants to merge 6 commits intomainfrom
color_scheme_dark member and themeable members (colors only)#1207Conversation
color_scheme_dark member and themeable members (colors only)
There was a problem hiding this comment.
Pull request overview
Adds a manifest-level mechanism to specify dark-mode overrides for color-related members, addressing the need to provide different theme/background colors for light vs. dark color schemes (Issue #975).
Changes:
- Marks
theme_colorandbackground_coloras “themeable members”. - Introduces a new
color_scheme_darkmanifest member (ordered map) to hold dark-theme overrides. - Adds a processing algorithm for
color_scheme_darkand hooks it into the manifest processing steps.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
index.html
Outdated
| The [=manifest's=] <code><dfn data-export="" data-dfn-for= | ||
| "manifest">color_scheme_dark</dfn></code> member is an [=ordered | ||
| map=] whose keys are the names of [=themeable members=], while the | ||
| values are their corresponding theme overrides. |
There was a problem hiding this comment.
"theme overrides" is introduced here but not defined elsewhere in the document. Consider either defining what a "theme override" is (including its value type) or rephrasing to state that the values are the same types as the corresponding members (e.g., color strings) and are processed with the existing color-member processing algorithm.
| values are their corresponding theme overrides. | |
| values are their corresponding <dfn>theme overrides</dfn>. A theme | |
| override is a value of the same type as its corresponding | |
| [=themeable member=] (for example, a color string for a color | |
| member) and is processed using that member's existing processing | |
| algorithm. |
There was a problem hiding this comment.
If we do this, we could avoid having the algorithm below. But - I'm generally a fan of algorithms in spec.....
One idea without the algorithm, we can:
- When we define a 'themeable member', we can say the member may be changed based the color theme of the operating system.
- We define "color_scheme_dark" as a "theme"
- When parsing a 'theme', kind of like *_Localized, we apply these values on top of the manifest json if that theme is being used. This makes it so we don't have to define parsing of the fields.
What do you think?
dmurph
left a comment
There was a problem hiding this comment.
I think the current version should 'work' - but interested in your thoughts on alternative design here.
Sorry for review delay - was trying to think about the alternative way here.
If you prefer this way, then we can keep it. Happy to hear others thoughts though
index.html
Outdated
| The [=manifest's=] <code><dfn data-export="" data-dfn-for= | ||
| "manifest">color_scheme_dark</dfn></code> member is an [=ordered | ||
| map=] whose keys are the names of [=themeable members=], while the | ||
| values are their corresponding theme overrides. |
There was a problem hiding this comment.
If we do this, we could avoid having the algorithm below. But - I'm generally a fan of algorithms in spec.....
One idea without the algorithm, we can:
- When we define a 'themeable member', we can say the member may be changed based the color theme of the operating system.
- We define "color_scheme_dark" as a "theme"
- When parsing a 'theme', kind of like *_Localized, we apply these values on top of the manifest json if that theme is being used. This makes it so we don't have to define parsing of the fields.
What do you think?
- Define themeable member by enumeration (<ul>) rather than circular
prose ("can be themed"), making the set explicit and extensible
- Replace hardcoded algorithm steps with a loop over the member set
- Add normative applying text: specifies selection logic when OS uses
a dark color theme, covering the dark-only case implicitly
- Remove undefined "theme overrides" term from prose
- Replace "dark mode" with "dark color theme" / "operating system's
color theme preference" throughout, consistent with MQ5 framing
- Scope themeable member dfn to manifest/ and update all link sites
- Remove redundant subsection prose that restated the definition
- Inline "is a themeable member" into theme_color and background_color member definitions, removing standalone paragraphs - Move SHOULD applying text closer to the themeable member definition - Explain SHOULD via "unless" clause (user preferences/accessibility) - Remove historical justification note - Resolve repeated-entries concern (moot with fixed-set loop)
|
Hi @christianliebel and @dmurph — I've pushed a few commits on top of yours addressing the review comments. Here's a summary of what changed: Spec design changes:
xref/markup fixes:
On @dmurph's Let me know if anything looks off. |
|
Thanks @marcoscaceres, LGTM! |
Closes #975
This change (choose at least one, delete ones that don't apply):
Implementation commitment (delete if not making normative changes):
If change is normative, and it adds or changes a member:
Commit message:
Add
color_scheme_darkmember and themeable membersPerson merging, please make sure that commits are squashed with one of the following as a commit message prefix:
Preview | Diff