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

style: run pretteier on web/css batch 1 #20691

Merged
merged 2 commits into from
Sep 25, 2022
Merged

Conversation

nschonni
Copy link
Contributor

Discarded a few changes for thins like overlow where the formatting would have gone agains the example intent, and some mixed good/bad samples that fixed parts that were explicitly wrong

@nschonni nschonni requested a review from a team as a code owner September 13, 2022 18:59
@nschonni nschonni requested review from estelle and removed request for a team September 13, 2022 18:59
@github-actions github-actions bot added the Content:CSS Cascading Style Sheets docs label Sep 13, 2022
@github-actions

This comment was marked as resolved.

Copy link
Member

@estelle estelle left a comment

Choose a reason for hiding this comment

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

There are a few places where the original was better.

@@ -41,8 +41,7 @@ Try typing in the field below. If your browser supports this property, the chara
#### HTML

```html
<label for="name">Name:</label>
<input type="text" name="name" id="name" />
<label for="name">Name:</label> <input type="text" name="name" id="name" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<label for="name">Name:</label> <input type="text" name="name" id="name" />
<label for="name">Name:</label>
<input type="text" name="name" id="name" />

Copy link
Member

Choose a reason for hiding this comment

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

example code is more legible on two lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm not sure how to make Prettier not do this. Maybe using label wrapping might make it more controlable

src: local(Example Font),
url('examplefont.woff') format("woff"),
url('examplefont.otf') format("opentype");
src: local(Example Font), url("examplefont.woff") format("woff"), url("examplefont.otf")
Copy link
Member

Choose a reason for hiding this comment

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

The original is better. It's also what the page is teaching, so it should be in the most legible format.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was decided to mark these with css-nolint later.

Copy link
Member

@teoli2003 teoli2003 Sep 14, 2022

Choose a reason for hiding this comment

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

More precisely, it was decided to revisit them later. Once we have fixed all the non-controversial ones, or the ones that can be easily fixed, we can focus on the problematic ones and see how to fix them.

I don't want to put css-nolint everywhere.

Maybe we will not run Prettier automatically, but only locally, followed by manual correction.

Honestly, I don't know yet.

local(Example Font-Bold), /* postscript name */
url('examplefont.woff') format("woff"),
url('examplefont.otf') format("opentype");
src: local(Example Font Bold), /* full font name */ local(Example Font-Bold), /* postscript name */
Copy link
Member

Choose a reason for hiding this comment

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

original is better. As it is what this page is teaching, it must be kept.

@import url supports( supports-query );
@import url supports( supports-query ) list-of-media-queries;
@import url supports(supports-query);
@import url supports(supports-query) list-of-media-queries;
Copy link
Member

Choose a reason for hiding this comment

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

I find the original more legible.
supports-query is not the actual parameter, but, rather, a value type.

@import url layer( layer-name ) list-of-media-queries;
@import url layer( layer-name ) supports( supports-query ) list-of-media-queries;
@import url layer(layer-name);
@import url layer(layer-name) list-of-media-queries;
Copy link
Member

Choose a reason for hiding this comment

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

same here

(min-resolution: 2dppx),
/* dppx fallback */
(min-resolution: 192dpi);
/* Older Firefox browsers (prior to firefox 16) */ (min--moz-device-pixel-ratio: 2),
Copy link
Member

Choose a reason for hiding this comment

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

original is better.
If we must go on one line, put comment after the code.

@@ -34,8 +34,7 @@ This example creates a small checkbox for users with fine primary pointers and a
### HTML

```html
<input id="test" type="checkbox" />
<label for="test">Look at me!</label>
<input id="test" type="checkbox" /> <label for="test">Look at me!</label>
Copy link
Member

Choose a reason for hiding this comment

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

more legible on two lines

media="screen and (shape: rect)"
rel="stylesheet"
href="rectangle.css" />
<link media="screen and (shape: round)" rel="stylesheet" href="round.css" />
Copy link
Member

Choose a reason for hiding this comment

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

why are these two different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think it's do do with the first hitting a line length that caused it to break on the attributes instead of a single line. I think there is an option to force the previous style, but it doesn't seem like overrides are the way this repo wants to go for now

Copy link
Member

@estelle estelle left a comment

Choose a reason for hiding this comment

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

Gave official suggestions for the things i had issues with so they can either be approved or rejected and we can move forward with a merge sooner rather than later..

files/en-us/web/css/@font-face/src/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/@font-face/src/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/@import/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/@import/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/@import/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/@media/pointer/index.md Outdated Show resolved Hide resolved
files/en-us/web/css/@media/shape/index.md Outdated Show resolved Hide resolved
nschonni and others added 2 commits September 20, 2022 18:30
Co-authored-by: Estelle Weyl <estelle@openwebdocs.org>
@estelle estelle merged commit c18a696 into mdn:main Sep 25, 2022
@nschonni nschonni deleted the prettier-web-css-1 branch September 25, 2022 16:56
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
* style: run pretteier on web/css

* Apply suggestions from code review

Co-authored-by: Estelle Weyl <estelle@openwebdocs.org>

Co-authored-by: Estelle Weyl <estelle@openwebdocs.org>
ferdnyc added a commit to ferdnyc/content-1 that referenced this pull request Jul 5, 2024
A `format();` argument was accidentally doubled when applying
suggested changes during review of mdn#20691.
Josh-Cena added a commit that referenced this pull request Jul 6, 2024
* CSS(src): Fix invalid example @font-face def

A `format();` argument was accidentally doubled when applying
suggested changes during review of #20691.

* Update index.md

* Update index.md

* Update index.md

* Update index.md

---------

Co-authored-by: Joshua Chen <sidachen2003@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:CSS Cascading Style Sheets docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants