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: add system theme option #586

Merged

amareshsm merged 14 commits into eslint:main from akulsr0:akul/add-system-theme-option

Jul 24, 2024

Contributor

akulsr0 commented Jun 18, 2024 •

edited

Loading

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request?

Closes eslint/eslint#18190

What changes did you make? (Give an overview)

Add System button for theme switcher

Related Issues

Is there anything you'd like reviewers to focus on?

Translations, I've added by searching on google translate, can be double checked by reviewers.


          feat: add system theme option

eslint-github-bot bot added the feature label

github-actions bot added the translation label

netlify bot commented Jun 18, 2024 •

edited

Loading

✅ Deploy Preview for hi-eslint ready!

Name	Link
🔨 Latest commit	`4c769b4`
🔍 Latest deploy log	https://app.netlify.com/sites/hi-eslint/deploys/669b66bbb7c3bc0008f0deaf
😎 Deploy Preview	https://deploy-preview-586--hi-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Jun 18, 2024 •

edited

Loading

✅ Deploy Preview for es-eslint ready!

Name	Link
🔨 Latest commit	`4c769b4`
🔍 Latest deploy log	https://app.netlify.com/sites/es-eslint/deploys/669b66bbafe6d50008686f23
😎 Deploy Preview	https://deploy-preview-586--es-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Jun 18, 2024 •

edited

Loading

✅ Deploy Preview for new-eslint ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`4c769b4`
🔍 Latest deploy log	https://app.netlify.com/sites/new-eslint/deploys/669b66bbaf6f6a0008660a67
😎 Deploy Preview	https://deploy-preview-586--new-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Jun 18, 2024 •

edited

Loading

✅ Deploy Preview for zh-hans-eslint ready!

Name	Link
🔨 Latest commit	`4c769b4`
🔍 Latest deploy log	https://app.netlify.com/sites/zh-hans-eslint/deploys/669b66bbc69d9f0008ecf6c4
😎 Deploy Preview	https://deploy-preview-586--zh-hans-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Jun 18, 2024 •

edited

Loading

✅ Deploy Preview for pt-br-eslint ready!

Name	Link
🔨 Latest commit	`4c769b4`
🔍 Latest deploy log	https://app.netlify.com/sites/pt-br-eslint/deploys/669b66bb306cf400085dcd68
😎 Deploy Preview	https://deploy-preview-586--pt-br-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Jun 18, 2024 •

edited

Loading

✅ Deploy Preview for fr-eslint ready!

Name	Link
🔨 Latest commit	`4c769b4`
🔍 Latest deploy log	https://app.netlify.com/sites/fr-eslint/deploys/669b66bbc2d39a0008ea9a14
😎 Deploy Preview	https://deploy-preview-586--fr-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Jun 18, 2024 •

edited

Loading

✅ Deploy Preview for ja-eslint ready!

Name	Link
🔨 Latest commit	`4c769b4`
🔍 Latest deploy log	https://app.netlify.com/sites/ja-eslint/deploys/669b66bb2bdcf000080d8e4c
😎 Deploy Preview	https://deploy-preview-586--ja-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify bot commented Jun 18, 2024 •

edited

Loading

✅ Deploy Preview for de-eslint ready!

Name	Link
🔨 Latest commit	`4c769b4`
🔍 Latest deploy log	https://app.netlify.com/sites/de-eslint/deploys/669b66bbf3b5d80008bddb10
😎 Deploy Preview	https://deploy-preview-586--de-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

Contributor

snitin315 commented Jun 18, 2024

@akulsr0 The npm run build script is failing, can you fix it?

akulsr0 added 2 commits

June 18, 2024 16:20


          chore: add translations for system word

ea91f79


          chore: handle playground theming

8eb2365

Contributor Author

akulsr0 commented Jun 20, 2024

@snitin315 This is fixed, translation was missing for other languages, added it.

amareshsm added the accepted label

amareshsm reviewed

View reviewed changes

src/_includes/layouts/base.html Outdated Show resolved Hide resolved

amareshsm reviewed

View reviewed changes

src/assets/js/themes.js Outdated Show resolved Hide resolved

Member

amareshsm commented Jun 20, 2024 •

edited

Loading

Thank you for the PR.

Here are a few suggestions:

Instead of adding a new key system-theme, use the existing theme key and set its value to "system" when the user selects the system theme.
The data-system-theme attribute is unnecessary.
In tablet view, the new system button causes the language selection option to be cropped in the UI.

Pls refer to this PR eslint/eslint#18617 I have made the changes for https://eslint.org/docs/

akulsr0 added 2 commits

June 21, 2024 07:56


          refactor: remove seperate key for system theme

fe4771c


          refactor: remove unintended semicolon

8a3892a

harish-sethuraman reviewed

View reviewed changes

src/_includes/components/theme-switcher.html Outdated Show resolved Hide resolved

akulsr0 added 2 commits

June 21, 2024 12:03


          chore: add aria-hidden=true to svg icon

5554efe


          chore: add aria-hidden=true to svg icon

f41924d

Tanujkanti4441 reviewed

View reviewed changes

Contributor

Tanujkanti4441 left a comment

the footer is still going out of the screen can you fix this also?

akulsr0 added 3 commits

June 24, 2024 10:16


          fix: footer language option getting out of screen

f2cabb3


          Merge branch 'main' into akul/add-system-theme-option

cc86347


          refactor: formatting fix

bf8f2ac

nzakas added the contributor pool label

Member

nzakas commented Jul 12, 2024

@Tanujkanti4441 @harish-sethuraman @amareshsm it looks like there have been updates since your last comments. Can you please re-check?

Member

amareshsm commented Jul 12, 2024

@Tanujkanti4441 @harish-sethuraman @amareshsm it looks like there have been updates since your last comments. Can you please re-check?

Thanks for the reminder. I'll re-check.

Member

amareshsm commented Jul 13, 2024 •

edited

Loading

The functionality works as expected, but there are redundancies in the theme.js file that need to be removed. Refer to this commit - eslint/eslint@7e724bb

In the playground, the theme option is going out of the screen


          refactor: pr review changes

61752f8

amareshsm reviewed

View reviewed changes

src/assets/scss/components/theme-switcher.scss Outdated Show resolved Hide resolved

amareshsm reviewed

View reviewed changes

src/assets/scss/components/theme-switcher.scss Outdated Show resolved Hide resolved


          chore: theme switcher padding fix

f2823df

amareshsm reviewed

View reviewed changes

src/playground/components/ThemeSwitcher.js Outdated Show resolved Hide resolved


          refactor: pr review changes

8adcb8d

amareshsm reviewed

View reviewed changes

Member

amareshsm left a comment

At 800px, the select option is being cut off and is not fully visible on the screen.


          fix: lang select cutting

4c769b4

harish-sethuraman approved these changes

View reviewed changes

Member

harish-sethuraman left a comment

LGTM. Will wait for other reviews to be closed

amareshsm approved these changes

View reviewed changes

Member

amareshsm left a comment

Thanks 🎉, LGTM.

amareshsm merged commit 7def673 into eslint:main

37 checks passed

zhangenming pushed a commit to zhangenming/eslint.org that referenced this pull request


          feat: add system theme option (eslint#586)

3d4e863

* feat: add system theme option

* chore: add translations for system word

* chore: handle playground theming

* refactor: remove seperate key for system theme

* refactor: remove unintended semicolon

* chore: add `aria-hidden=true` to svg icon

* chore: add `aria-hidden=true` to svg icon

* fix: footer language option getting out of screen

* refactor: formatting fix

* refactor: pr review changes

* chore: theme switcher padding fix

* refactor: pr review changes

* fix: lang select cutting

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

Labels

accepted contributor pool feature translation