Skip to content

[TwigBridge] Provide a default non-empty label on empty options to validate HTML5 #44464

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
Merged
merged 1 commit into from
Oct 24, 2022
Merged

[TwigBridge] Provide a default non-empty label on empty options to validate HTML5 #44464

merged 1 commit into from
Oct 24, 2022

Conversation

DocFX
Copy link
Contributor

@DocFX DocFX commented Dec 5, 2021 •
edited by nicolas-grekas
Loading

Q A
Branch? 6.2
Bug fix? no
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

As described on https://rocketvalidator.com/html-validation/element-option-without-attribute-label-must-not-be-empty, so far this template generates empty options with no value, no content, and no label. This is not a break, but a simple "should".

Not sure how we should recommend handling this, users have the possibility to handle this themselves (https://symfony.com/doc/current/reference/forms/types/choice.html#placeholder), but if they don't we leave a non-stadanrd-compliant output.

The tailwind template is also affected from this, as it extends this one.

I proposed a double-dash default value:

  • pretty affordant
  • doesn't use translations as users might not use them
  • doesn't suffer from language dependent doubtful meaning

I locally fix this by overriding the default templates, but I thought the default ones could get a fix.

Also, to make it quick, I proposed to change this starting 5.4. Maybe we can push this back to 4.4?

@carsonbot carsonbot added this to the 5.4 milestone Dec 5, 2021
@DocFX DocFX changed the title Provide a default non-empty label on empty options to validate HTML5 [TwigBridge] Provide a default non-empty label on empty options to validate HTML5 Dec 5, 2021
@carsonbot
Copy link

Hey!

I think @VincentLanglet has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@DocFX
Copy link
Contributor Author

DocFX commented Dec 9, 2021

Don't really feel like I broke any test, here. ^^

@stof
Copy link
Member

stof commented Dec 9, 2021

should this label attribute be set when there is a placeholder provided ? Or should the -- be the default text of the option instead ?

@DocFX
Copy link
Contributor Author

DocFX commented Dec 10, 2021

That's an excellent question, @stof. My understanding so far is that the placeholder should provide a value, hence removing the need for the label attribute. (if I read everything correctly)

@fabpot
Copy link
Member

fabpot commented Jul 21, 2022

Not sure about this change, but that would be for 6.2.

@DocFX
Copy link
Contributor Author

DocFX commented Jul 28, 2022

Haha I totally forgot about that PR. I just had to re-read it. XD

Was just an ultra-low-priority suggestion.

I can delete this and do a new one on 6.2, if not already done.

nicolas-grekas
nicolas-grekas reviewed Sep 28, 2022
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Can you please rebase for 6.2?
Also, why not go with : '--' on the ternary op inside the tag? That'd make more sense to me.

@nicolas-grekas nicolas-grekas modified the milestones: 5.4, 6.2 Sep 28, 2022
@DocFX
Copy link
Contributor Author

DocFX commented Oct 2, 2022 •
edited
Loading

Can you please rebase for 6.2?

Done! :)

Also, why not go with : '--' on the ternary op inside the tag? That'd make more sense to me.

My brain is in slow mode today (rhinopharyngitis), not sure I understand what you mean ^^

@DocFX DocFX changed the base branch from 5.4 to 6.2 October 18, 2022 13:58
@nicolas-grekas nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 19, 2022
@carsonbot carsonbot changed the title [TwigBridge] Provide a default non-empty label on empty options to validate HTML5 Provide a default non-empty label on empty options to validate HTML5 Oct 19, 2022
@carsonbot carsonbot changed the title Provide a default non-empty label on empty options to validate HTML5 [TwigBridge] Provide a default non-empty label on empty options to validate HTML5 Oct 24, 2022
@nicolas-grekas
Copy link
Member

Thank you @DocFX.

@nicolas-grekas nicolas-grekas merged commit 96e7ae7 into symfony:6.2 Oct 24, 2022
nicolas-grekas added a commit that referenced this pull request Oct 24, 2022
@nicolas-grekas
Revert "minor #44464 [TwigBridge] Provide a default non-empty label o…
88191f5 
…n empty options to validate HTML5 (DocFX)"

This reverts commit 96e7ae7, reversing
changes made to f6fb96a.
@nicolas-grekas
Copy link
Member

I merged to fast. This breaks tests so I reverted in 88191f5
Please resubmit if you think we should follow up.

@DocFX
Copy link
Contributor Author

DocFX commented Oct 28, 2022

Oh yeah, sorry, the first time I opened it the tests were failing for external reasons.

This was referenced Oct 28, 2022
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@lyrixx lyrixx Awaiting requested review from lyrixx

@yceruto yceruto Awaiting requested review from yceruto

@wouterj wouterj Awaiting requested review from wouterj

@chalasr chalasr Awaiting requested review from chalasr

@dunglas dunglas Awaiting requested review from dunglas

@OskarStark OskarStark Awaiting requested review from OskarStark

@jderusse jderusse Awaiting requested review from jderusse

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Needs Review TwigBridge
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.
5 participants
@DocFX @carsonbot @stof @fabpot @nicolas-grekas