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

Reduce “HTML attribute: required” example to conforming HTML markup #21036

Conversation

sideshowbarker
Copy link
Collaborator

Fixes #20597

@sideshowbarker sideshowbarker requested a review from a team as a code owner September 24, 2022 09:12
@sideshowbarker sideshowbarker requested review from schalkneethling and removed request for a team September 24, 2022 09:12
@github-actions github-actions bot added the Content:HTML Hypertext Markup Language docs label Sep 24, 2022
@github-actions
Copy link
Contributor

Preview URLs

Flaws (3)

URL: /en-US/docs/Web/HTML/Attributes/required
Title: HTML attribute: required
Flaw count: 3

  • macros:
    • /en-US/docs/Web/API/ValidityState/valueMissing does not exist
    • /en-US/docs/Web/CSS/validityState.valueMissing does not exist
  • bad_bcd_queries:
    • No BCD data for query: html.elements.attributes.required

@@ -49,14 +49,14 @@ Provide an indication to users informing them the form control is required. Ensu
```html
<form>
<div class="group">
<input type="text" />
<input type="text">
Copy link
Member

Choose a reason for hiding this comment

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

@sideshowbarker The trailing slash is automatically added by Prettier. It's purely stylistic and we don't want to change it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sideshowbarker The trailing slash is automatically added by Prettier. It's purely stylistic and we don't want to change it.

That’s very disappointing to hear, and that’s very bad misfeature of Prettier. We should fix that bug in our Prettier config. Using self-closing tag syntax in text/html content is a big antipattern and completely against best practice.

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 trailing slash a bit easier to understand since I, being not well-versed in HTML, don't need to lookahead for the closing tag and end up realizing it's a void tag. That's also the rationale for Prettier I believe.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason that we decided to go for adding the trailing slashes to self-closing tags is a combination of two main things:

First, adding the trailing slashes improves readability, especially for those getting started with HTML. Having a trailing slash better shows that the tag is closing itself, thus the developer does not have to add their own closing tag. A beginner may not know which tags are self-closing and which aren't, and even a more advanced user may forget at times.

Second, this is something that Prettier adds and is not configurable. Prettier does not offer the option to disable this, probably for the above reason as well. If we wanted to disable this behavior, we would end up having to create our own Prettier fork (since they don't wish to add more configuration options), and I don't think we should for something that would most likely cause more learning conflict.

If you'd like, @sideshowbarker, we could open up a discussion for this to collect more feedback and weigh the pros and cons better!


By the way, why do you say this is bad practice?

<label>Normal</label>
</div>
<div class="group">
<input type="text" required="required" />
<input type="text" required>
Copy link
Member

Choose a reason for hiding this comment

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

@sideshowbarker Isn't this a legitimate improvement though?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sideshowbarker Isn't this a legitimate improvement though?

Yeah replacing required="required" with required is a good change, so somebody else should go ahead and make that change. But I personally can no longer make any fixes to markup snippets like this without also removing the self-closing tag syntax.

I don’t recall there ever being any discussion about changing all void elements in our HTML markup snippets to use self-closing tag syntax, and would never have voiced agreement to it if we had had such a discussion.

In general, it seems like a pretty bad idea to be having the default behavior of third-party pools driving decisions about our content. Tail wagging the dog.

Copy link
Member

Choose a reason for hiding this comment

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

Issue is here: #20523 We simply removed the requirement. In the last editorial call (which you unfortunately could not attend) we also reached some sort of consensus that it's a better thing to do because we absolutely want Prettier and we don't have strong opinions about aligning with how everyone else using Prettier is formatting their markup.

Copy link
Collaborator Author

@sideshowbarker sideshowbarker Sep 25, 2022

Choose a reason for hiding this comment

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

Issue is here: #20523 We simply removed the requirement. In the last editorial call (which you unfortunately could not attend) we also reached some sort of consensus that it's a better thing to do because we absolutely want Prettier and we don't have strong opinions about aligning with how everyone else using Prettier is formatting their markup.

Thanks for the pointer. It’s pretty disappointing that this choice got made.

Is it really true that it’s not possible to make Prettier use self-closing tag syntax for all void elements?

If so, then I think that makes Prettier a broken tool for HTML formatting purposes, and we should stop using it on our HTML snippets. It’s really not clear to me how much value we’re getting out of using it on HTML snippets anyway.

And certainly when we have to start deleting best-practices guidance from our project style guide because we’ve chosen to use some third-party tool that doesn’t conform to our existing style guidelines, I’d think that really ought to kind of raise a big red flag for everybody.

Copy link
Member

Choose a reason for hiding this comment

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

Could you raise a discussion about this with your opinions? Because frankly there are strong reasons about doing it otherwise (e.g. less cognitive overload about whether a closing tag exists somewhere), and the only counter-reason I can think of is "because HTML4 didn't allow it".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I can’t justify taking time to re-open discussion about it. But as I mentioned earlier, one consequence of this FWIW is that for markup snippets like this one that have self-closing tag syntax and that come up in an issue or patch, I’m not personally going to be able to work on the related issue or review any related patch without also removing the self-closing tag syntax.

Copy link
Member

@Josh-Cena Josh-Cena Sep 25, 2022

Choose a reason for hiding this comment

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

OK, I respect your opinion on this 😄 Relevant thread, FYI: prettier/prettier#5246

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I respect your opinion on this 😄 Relevant thread, FYI: prettier/prettier#5246

Thanks for the the pointer. And I very much respect your opinion as well 😄 After reading that thread, I understand more about the Prettier philosophy, but the fact that issue — which is asking just for an option to not use self-closing — has 213 thumbs-ups and been open for 3 years, yet the effective response from the maintainers is apparently “we don’t do options; we’re an opionated tool; if you don’t like something that Prettier does for some particular language, then just don’t use Prettier for that language” doesn’t leave me terrifically impressed with how important decisions get made by the Prettier project leadership, and how responsive they are to the wider community.

Especially given the multiple style guides cited in that issue (of which our own style guide would/could have been one before we scrubbed the existing guidance from it because Prettier behavior conflicts with it) which state that self-closing tag syntax shouldn’t be used, it seems like, without having buy-in from the wider community of users — and in fact in the face of a significant number of users from the community stepping forward to say they need an option to disable it, but the maintainers choosing not to provide such an option — this behavior in Prettier is effectively just an arbitrary judgement call on the part of Prettier maintainers

And so it seems to me that by choosing to use Prettier on our HTML snippets, we’re effectively also letting the Prettier maintainers make that judgement call for us too. I understand that there was MDN project discussion about it after #20523 was raised and I understand and respect that seeing self-closing tags as preferable is one reasonable point of view — but it still looks like the biggest reason driving the removal of the relevant guidance from our style guide on this is just the desire to be able to use Prettier on our HTML snippets, and thus the need for acquiescence to the judgement calls of the Prettier maintainers.

@sideshowbarker sideshowbarker deleted the sideshowbarker/required-attribute-markup-use-best-practice branch September 25, 2022 01:12
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Required="required" VS required
3 participants