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

fix(css): add nolint and increase output heights #37255

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar commented Dec 18, 2024

Increase live sample output heights to avoid scrolling. Also, add nolint to the code block to prevent weird formatting.

/cc @chrisdavidmills

@OnkarRuikar OnkarRuikar requested a review from a team as a code owner December 18, 2024 06:02
@OnkarRuikar OnkarRuikar requested review from estelle and removed request for a team December 18, 2024 06:02
@github-actions github-actions bot added Content:HTML Hypertext Markup Language docs size/s [PR only] 6-50 LoC changed labels Dec 18, 2024
Copy link
Contributor

Preview URLs

@Josh-Cena
Copy link
Member

I'm a personal -1 on disabling the formatter here. -nolint is meant for cases where the code must not be formatted, such as syntax blocks or purposefully demonstrating whitespace differences. If we just subjectively decide that "because the formatting is not good enough here, let's not format it", then we are in a worse state than not having a formatter at all, because now we have even more bikeshedding than before: do we nolint or not? If we do, then how do we format it? The goal of a formatter is not to make something everyone finds beautiful, but something that's both predictable and doesn't change program semantics. I don't feel strongly enough to block this PR, but it opens a path I'm not happy for us to go down.

@OnkarRuikar
Copy link
Contributor Author

This is not the first case. Many CSS and HTML code blocks have been marked -nolint (from the start) because manual formatting looks way better.

nolint is meant for cases where the code must not be formatted, such as syntax blocks or purposefully demonstrating whitespace differences.

Out of 4k+ -nolints in the repo, you'll find very few fit this criteria.

@Josh-Cena
Copy link
Member

This is not to say these cases are fair either, but their existence is why my opinion is not strong enough to fully block this PR. If you search by (?<!## Syntax\n\n)```\w+-nolint(?! example-bad), you'll find significantly fewer cases, many of which are still either actually unformattable JS snippets or SVG disguised as HTML so they get rendered by live examples.

@OnkarRuikar
Copy link
Contributor Author

This is not to say these cases are fair either,

For a better reader experience, we need to make some exceptions.

Copy link
Contributor

@chrisdavidmills chrisdavidmills left a comment

Choose a reason for hiding this comment

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

Thanks, @OnkarRuikar, that's much better!

@chrisdavidmills chrisdavidmills merged commit 0d945ca into mdn:main Dec 18, 2024
7 checks passed
@OnkarRuikar OnkarRuikar deleted the css_summary branch December 19, 2024 02:34
allan-bonadio pushed a commit to allan-bonadio/content that referenced this pull request Dec 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:HTML Hypertext Markup Language docs size/s [PR only] 6-50 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants