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 language option styling #16636

Closed
wants to merge 1 commit into from

Conversation

amareshsm
Copy link
Member

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 language option styling.

What changes did you make? (Give an overview)

Due to the style change (pre[class*="language-"] to div[class*="language-"] - syntax-highlighter.scss) in #16606, language option styles were affected. Fixed by excluding the .language-switcher class.

Before After
image image
image image

@amareshsm amareshsm requested a review from a team as a code owner December 10, 2022 10:36
@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon documentation Relates to ESLint's documentation labels Dec 10, 2022
@netlify
Copy link

netlify bot commented Dec 10, 2022

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit d86c3f5
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/63946145a982750008ee7b36
😎 Deploy Preview https://deploy-preview-16636--docs-eslint.netlify.app/user-guide/command-line-interface
📱 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 settings.

@harish-sethuraman
Copy link
Member

Wasn't it part of design? I thought it was part of design 😅

@amareshsm
Copy link
Member Author

Wasn't it part of design? I thought it was part of design 😅

No, it's not a part of the design.

Copy link
Member

@harish-sethuraman harish-sethuraman left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@chenxsan
Copy link
Contributor

Absolutely that's a visual regression my pr introduced.

@amareshsm amareshsm added accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Dec 10, 2022
@nzakas
Copy link
Member

nzakas commented Dec 13, 2022

Can we change this line to use “pre” instead of “div” and then revert the CSS?
https://github.com/eslint/eslint/pull/16606/files#diff-0e154e64478403222a01a7cb42c83ab07a2e01a90f65b5d0839b922570d4b03aR65

@chenxsan
Copy link
Contributor

Can we change this line to use “pre” instead of “div” and then revert the CSS? https://github.com/eslint/eslint/pull/16606/files#diff-0e154e64478403222a01a7cb42c83ab07a2e01a90f65b5d0839b922570d4b03aR65

Hi Nicholas, I'm afraid we can't simply change that and revert the respective css changes. The default fence rule of markdown-it will always return <pre><code></code></pre>. Replacing that div with pre will result in a nested pre like

<pre>
  <pre><code/></pre>
</pre>

which would introduce another styling problem.

It's possible to keep the original css class if I didn't implement the div wrapper in the beginning. Let me know if you want me to file a pull request to update it or prefer to merge this fix.

@nzakas
Copy link
Member

nzakas commented Dec 14, 2022

Ah I see. I'm just not a fan of using :not() to fix this kind of an error -- we should have unique and non-overlapping CSS class names for the two different use cases like we did before.

@chenxsan
Copy link
Contributor

Ah I see. I'm just not a fan of using :not() to fix this kind of an error -- we should have unique and non-overlapping CSS class names for the two different use cases like we did before.

Class name collisions seem inevitable when they're semantic :), especially when we have many contributors in open source projects.

How about renaming language-switcher to locale-switcher, which would require a few more changes than the current fix in this pull request:

@nzakas
Copy link
Member

nzakas commented Dec 15, 2022

Class name collisions seem inevitable when they're semantic :), especially when we have many contributors in open source projects.

Yes but we didn't have any until this change. :) Managing exclusions tends to cause errors in the long-run because you need to keep track of them.

Can't we just add the language- class here and have it work like it used to?

@chenxsan
Copy link
Contributor

Class name collisions seem inevitable when they're semantic :), especially when we have many contributors in open source projects.

Yes but we didn't have any until this change. :) Managing exclusions tends to cause errors in the long-run because you need to keep track of them.

Can't we just add the language- class here and have it work like it used to?

Sure. That was my plan when I said above I can file another pull request to update it:

@harish-sethuraman
Copy link
Member

@amareshsm shall we close this in favour of #16669 I think both of the PRs are solving the same problem?

@amareshsm
Copy link
Member Author

@amareshsm shall we close this in favour of #16669 I think both of the PRs are solving the same problem?

yes once the PR is ready we check and close this PR.

@amareshsm
Copy link
Member Author

Closing this in favor of #16669

@amareshsm amareshsm closed this Dec 18, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 17, 2023
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 17, 2023
@amareshsm amareshsm deleted the fix-broken-style branch February 1, 2024 15:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants