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

docs: fix no-js dark theme bugs #18071

Closed
wants to merge 4 commits into from

Conversation

amareshsm
Copy link
Member

@amareshsm amareshsm commented Feb 1, 2024

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:
Fixed no-js (js disabled) dark theme bugs.
Ref: #16612

What changes did you make? (Give an overview)

When javascript is disabled and the prefers-color-scheme is dark
1. Code block texts are not visible.
2. Logo/ tag text not inverted based on the theme.
3. Image background color not applied

Before After
image image
image image
image image
image image
image image
image image
image image

Is there anything you'd like reviewers to focus on?
I have addressed the identified issues. Feel free to add if anything missing.

@eslint-github-bot eslint-github-bot bot added the documentation Relates to ESLint's documentation label Feb 1, 2024
Copy link

netlify bot commented Feb 1, 2024

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit e27fe98
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/65bd166ab3ca98000842f9a0
😎 Deploy Preview https://deploy-preview-18071--docs-eslint.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.

@amareshsm amareshsm self-assigned this Feb 1, 2024
@amareshsm amareshsm marked this pull request as ready for review February 1, 2024 19:04
@amareshsm amareshsm requested a review from a team as a code owner February 1, 2024 19:04
Copy link
Member

@kecrily kecrily left a comment

Choose a reason for hiding this comment

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

LGTM. But I remember some of the problems we were supposed to have fixed at one point, I don't know if it's an illusion or not 🤷

@kecrily kecrily added the accepted There is consensus among the team that this change meets the criteria for inclusion label Feb 2, 2024
@amareshsm
Copy link
Member Author

LGTM. But I remember some of the problems we were supposed to have fixed at one point, I don't know if it's an illusion or not 🤷

Yes, PR was raised but not merged it was not completely done at that time.
#16612

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

The functionality looks good to me on the preview deploy, thanks for getting this started up again! ✨

But, requesting changes that I think there might be a strategy that avoids code duplication.

Copy link
Contributor

Choose a reason for hiding this comment

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

[Refactor] This adds in a new set of styles that only get hit in dark mode, and may only be tested in production by users with JavaScript disabled. Even if many users are visiting the site with JS disabled, that's still an extra source of truth for these comments. IMO it's not great having an additional (so, at least 2 total) place to manage these styles.

Instead, would it be feasible to figure out why some of the existing styles aren't loading in? E.g. if dark-mode-only styles are only loaded by JS, maybe they can be loaded in with pure HTML/CSS instead?

Sorry if I missed a previous explanation. I looked through the past PR comments and didn't see anything. I also filed #18076 for tracking.

Copy link
Member Author

@amareshsm amareshsm Feb 3, 2024

Choose a reason for hiding this comment

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

Thank you for your feedback and suggestion Yes, you are correct this added a new set of styles that are only used when js is disabled and the preferred theme is dark.

An additional place to these styles I am getting it? do you mean a new file specifically for handling no-js mode?

Instead, would it be feasible to figure out why some of the existing styles aren't loading in? E.g. if dark-mode-only styles are only loaded by JS, maybe they can be loaded in with pure HTML/CSS instead?

There is no existing style targeting drak mode with js disabled which is why it falls back to default styles. The styles that are not applied are related to JS.

For example:
let's take the ESLint logo - it is an SVG element. The default style works well in light mode.

<svg class="brand-logo" width="203" height="58" viewBox="0 0 203 58" fill="none" xmlns="http://www.w3.org/2000/svg" role="img" aria-label="ESLint logo">
<path d="M46.5572 21.1093L34.0167 13.8691C33.7029 13.6879 33.3161 13.6879 33.0023 13.8691L20.4616 21.1093C20.148 21.2905 19.9543 21.6253 19.9543 21.9878V36.4681C19.9543 36.8304 20.148 37.1654 20.4616 37.347L33.0023 44.5871C33.3161 44.7684 33.7029 44.7684 34.0167 44.5871L46.5572 37.347C46.871 37.1657 47.0644 36.8306 47.0644 36.4681V21.9878C47.0641 21.6253 46.8707 21.2905 46.5572 21.1093Z" fill="#8080F2" id="logo-center" class="logo-component" />
<path d="M0.904381 27.7046L15.8878 1.63772C16.4321 0.695223 17.4375 0 18.5258 0H48.4931C49.5817 0 50.5873 0.695223 51.1316 1.63772L66.115 27.6471C66.6593 28.5899 66.6593 29.7796 66.115 30.7224L51.1316 56.5756C50.5873 57.5181 49.5817 58 48.4931 58H18.526C17.4377 58 16.4321 57.5326 15.8881 56.5899L0.90464 30.6944C0.359854 29.7522 0.359854 28.6471 0.904381 27.7046ZM13.3115 40.2393C13.3115 40.6225 13.5422 40.977 13.8744 41.1689L32.96 52.1803C33.2919 52.3719 33.7078 52.3719 34.0397 52.1803L53.1401 41.1689C53.4721 40.977 53.7043 40.6228 53.7043 40.2393V18.2161C53.7043 17.8327 53.4754 17.4785 53.1432 17.2866L34.0584 6.27513C33.7264 6.08327 33.3111 6.08327 32.9792 6.27513L13.8775 17.2866C13.5453 17.4785 13.3115 17.8327 13.3115 18.2161V40.2393V40.2393Z" fill="#4B32C3" class="logo-component" />
<path d="M86.6971 43.7102V14.2899H105.442V18.871H91.7826V26.6044H104.265V31.1855H91.7826V39.129H105.652V43.7102H86.6971Z" fill="#101828" class="logo-component" />
<path d="M118.919 44.2986C116.678 44.2986 114.688 43.9063 112.951 43.1218C111.242 42.3092 109.897 41.1464 108.916 39.6334C107.936 38.1203 107.445 36.271 107.445 34.0855V32.9928H112.447V34.0855C112.447 36.0189 113.035 37.4619 114.212 38.4145C115.389 39.3672 116.958 39.8435 118.919 39.8435C120.909 39.8435 122.408 39.4372 123.416 38.6247C124.425 37.8121 124.929 36.7614 124.929 35.4725C124.929 34.6039 124.691 33.9034 124.215 33.371C123.739 32.8107 123.038 32.3623 122.113 32.0261C121.217 31.6899 120.124 31.3677 118.835 31.0594L117.574 30.8073C115.641 30.359 113.96 29.7986 112.531 29.1261C111.13 28.4256 110.051 27.529 109.295 26.4363C108.538 25.3435 108.16 23.9145 108.16 22.1493C108.16 20.3841 108.58 18.871 109.421 17.6102C110.261 16.3493 111.452 15.3826 112.993 14.7102C114.534 14.0377 116.341 13.7015 118.415 13.7015C120.488 13.7015 122.338 14.0517 123.963 14.7522C125.588 15.4527 126.863 16.5034 127.787 17.9044C128.74 19.3053 129.216 21.0566 129.216 23.158V24.545H124.215V23.158C124.215 21.9532 123.977 20.9865 123.5 20.258C123.024 19.5295 122.352 18.9971 121.483 18.6609C120.614 18.3247 119.592 18.1566 118.415 18.1566C116.678 18.1566 115.361 18.4928 114.464 19.1652C113.568 19.8377 113.119 20.7904 113.119 22.0232C113.119 22.8078 113.315 23.4802 113.708 24.0406C114.128 24.573 114.73 25.0213 115.515 25.3855C116.327 25.7218 117.336 26.016 118.541 26.2681L119.802 26.5623C121.819 27.0107 123.584 27.5851 125.098 28.2855C126.611 28.958 127.787 29.8546 128.628 30.9754C129.497 32.0962 129.931 33.5532 129.931 35.3464C129.931 37.1116 129.469 38.6667 128.544 40.0116C127.647 41.3566 126.372 42.4073 124.719 43.1638C123.094 43.9203 121.161 44.2986 118.919 44.2986Z" fill="#101828" class="logo-component" />
<path d="M133.1 43.7102V14.2899H138.185V39.129H151.971V43.7102H133.1Z" fill="#101828" class="logo-component" />
<path d="M154.827 43.7102V22.9479H159.661V43.7102H154.827ZM157.223 20.3C156.354 20.3 155.598 20.0198 154.954 19.4595C154.337 18.871 154.029 18.1005 154.029 17.1479C154.029 16.1952 154.337 15.4387 154.954 14.8783C155.598 14.2899 156.354 13.9957 157.223 13.9957C158.148 13.9957 158.904 14.2899 159.493 14.8783C160.109 15.4387 160.417 16.1952 160.417 17.1479C160.417 18.1005 160.109 18.871 159.493 19.4595C158.904 20.0198 158.148 20.3 157.223 20.3Z" fill="#101828" class="logo-component" />
<path d="M164.525 43.7102V22.9479H169.275V25.8479H169.989C170.353 25.0633 171.012 24.3208 171.964 23.6203C172.917 22.9198 174.36 22.5696 176.293 22.5696C177.891 22.5696 179.305 22.9338 180.538 23.6623C181.771 24.3909 182.724 25.3995 183.396 26.6884C184.097 27.9773 184.447 29.5044 184.447 31.2696V43.7102H179.614V31.6479C179.614 29.9667 179.193 28.7198 178.353 27.9073C177.54 27.0667 176.377 26.6464 174.864 26.6464C173.155 26.6464 171.81 27.2208 170.83 28.3696C169.849 29.4904 169.359 31.1015 169.359 33.2029V43.7102H164.525Z" fill="#101828" class="logo-component" />
<path d="M196.449 43.7102C195.104 43.7102 194.025 43.3179 193.213 42.5334C192.428 41.7208 192.036 40.6281 192.036 39.2551V26.9406H186.614V22.9479H192.036V16.2652H196.869V22.9479H202.837V26.9406H196.869V38.4566C196.869 39.2971 197.262 39.7174 198.046 39.7174H202.207V43.7102H196.449Z" fill="#101828" class="logo-component" />
</svg>

For the dark mode styles specified explicitly based on the data-theme attribute (which gets added based on the user choice)

<style>
[data-theme="dark"] .logo-component {
fill: #fff;
}
[data-theme="dark"] #logo-center {
opacity: 0.6;
}
</style>

If JavaScript is turned off and the chosen theme is light, there is no problem as the default logo functions correctly. However, when the preferred mode is dark, the absence of styles for this scenario results in a fallback to the light mode style, leading to the issue. To resolve this, I have specifically included styles that target the dark theme when JavaScript is disabled.

Additionally, let's examine the behaviour of various elements in a dark theme when JavaScript is disabled. The code block background colour toggles flawlessly between dark and light modes even without JavaScript, Because property value is not hard-coded

pre[class*="language-"] {
background-color: var(--lightest-background-color);
}

Variable --lightest-background-color value is toggled based on the theme

light theme:

--lightest-background-color: var(--color-neutral-50);

dark theme:

--lightest-background-color: var(--color-neutral-800);

preferred theme dark and js disabled:

--lightest-background-color: var(--color-neutral-800);

Preferred them light and js disabled:

--lightest-background-color: var(--color-neutral-50);

Using variables we can solve this issue. Refer: #18077

I believe this provides clarity.

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense, thanks! +1 to #18077's approach, I'll approve over there.

@amareshsm amareshsm marked this pull request as draft February 4, 2024 20:38
@amareshsm amareshsm closed this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion documentation Relates to ESLint's documentation
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

3 participants