search

primer / css

Primer is GitHub's design system. This is the CSS implementation
https://primer.style/css
MIT License
12.55k stars 1.21k forks source link

[Documentation] Typos, formatting issues and other improvements on css/getting-started/theming #1485

Closed aidswidjaja closed 3 years ago

aidswidjaja commented 3 years ago

Describe the bug There are multiple typos / formatting issues on the css/getting-started/theming page

1. Typos

Replace

as an alternaitve to custom color variables, use auto variables if the results give enough constrast.

to read

as an alternative to custom color variables, use auto variables if the results give enough contrast.

(Emphasis added)

2. SCSS highlight.js does not work as expected

On GitHub's Markdown file previewer, the Markdown code block tagged scss appropriately provides colour formatting. On the Primer website however, this does not render any colour formatting and thus is inconsistent with the rest of the web page. Looking back at the Markdown source, it appears that code blocks are tagged css elsewhere (which does render correctly) even with the presence of scss variables, so this behaviour should be used instead.

image Pictured above: code block without syntax highlighting

image Pictured above: code block with syntax highlighting

3. Unclear description of attributes

Consider this statement:

Configure Primer CSS to use a certain theme by adding the following attributes:

  • data-color-mode with a value of either light or dark
  • either data-light-theme or data-dark-theme with a value of either light, dark, dark_dimmed

I believe this is unclear as to the workings of the attributes. This is evident by requiring further example/explanation.

I would personally reword this section to:

Configure Primer CSS to use a certain theme by modifying its attributes

  • Change the color mode by setting the data-color-theme attribute to either light or dark
  • Change the dark theme variant by setting the data-dark-theme attribute to either dark or dark_dimmed
  • There is currently only one light theme variant that is set by setting the data-light-theme attribute to light

To Reproduce See https://primer.style/css/getting-started/theming

Desktop (please complete the following information):

Smartphone (please complete the following information):

Additional context I am aware that menial issues/PRs such as these are typically frowned upon (and for that reason why I didn't immediately make a PR), however given that there were a number of improvements that could have been made to this specific page, and with a significant effect on the user experience, I believed that it was important to raise :)

I am happy to make the PR if desired. Thanks for your time!

simurai commented 3 years ago

@aidswidjaja Thanks for pointing out all the improvements.. started a PR here: https://github.com/primer/css/pull/1500

  1. Unclear description of attributes

Hmm.. yeah, this might be a bit tricky to explain because it can be confusing. But when enabling "Sync with system" (auto) mode in GitHub's theme settings it's possible to use a "dark" theme in light (day) mode or a "light" theme in dark (night) mode:

Screen Shot 2021-07-14 at 15 34 40

It even works like this:

<html data-color-mode="light" data-light-theme="dark_dimmed"> <!-- works -->
<html data-color-mode="dark" data-dark-theme="light"> <!-- works too -->

But maybe we could just use your version and not bother to explain these unexpected options? 🤔

aidswidjaja commented 3 years ago

Ah, so the mode is separate from the theme. I played around with the attributes a bit and I think we could add something like this in the docs to make it obvious how each attribute works and to encourage best practice usage.

Theming

Primer CSS offers multiple color themes for components and utilities. Colors change based on the color mode and active theme.

  • Color modes are intrinsically linked with OS color schemes and define what system-level color scheme is recognized
  • Themes are set in response to the value defined by the color mode

There are currently 3 themes (light, dark, dark_dimmed) to choose from. When nothing is specified, Primer CSS uses the light theme.

Set a theme

Configure Primer CSS to use a certain theme by adding the following attributes:

  • change the color mode by setting the data-color-theme attribute to light, dark or auto
  • change the dark theme variant by setting the data-dark-theme attribute to either dark or dark_dimmed
  • there is currently only one light theme variant available, which is set by setting the data-light-theme attribute to light

Setting the color mode to auto will sync to the OS's color mode. If you set the color mode to auto you'll need to define both a data-dark-theme and data-light-theme attribute.

The attributes can be added to any element, but ideally it should be added to the document root (<html>). Below is an example to use the dark_dimmed theme:

(code block examples goes here)

This would remove the "Sync to the system" section which has been more closely integrated to other areas of the theming docs.

Ironically while writing this up I did find yet another typo 😄

Change > The attributes can be added to any element, but ideally it should be added to the document root (``). Below an example to use the `dark_dimmed` theme: to > The attributes can be added to any element, but ideally it should be added to the document root (``). Below **is** an example to use the `dark_dimmed` theme:

Let me know what you think :)

simurai commented 3 years ago

Woah.. while you wrote the above comment, I just committed https://github.com/primer/css/pull/1500/commits/298eaf3250dba31bfb57d3bb4bc725b4490abe5a. It basically puts the focus on the theme that should be used, then just shows what attributes to add. Does that make it easier?

aidswidjaja commented 3 years ago

Oh that does work great! It explains it clearer too which is good.

I'm not sure how you use GitHub theming internally and what your extensibility plans for it are later, but I think this is sufficient to the standard end user for now, particularly because there are only 3 themes currently. It's easy enough to work with programmatically with as well.

Thanks for your help with this! We can close this issue once #1500 is merged