-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
allows to set empty html attr #1548
Conversation
as for now there was no way to set empty attribute like required or autofocus
better to use is_bool here (instead of my previous solution which was $value !== false) , that way we left more flexibility for users for pre/post process attributes
Looks good, but we need some tests added to test that feature please. |
null, false, true, string, empty string, 0, 1
@lonnieezell : done. |
public function testFormParseFormAttributesFalse() | ||
{ | ||
$expected = 'disabled '; | ||
$this->assertEquals($expected, parse_form_attributes(['disabled' => false], [])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't a false setting on a value make it so that it does not appear in the attributes list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we have more flexibility outside form helper function if we will treat true as well as false in the same way.
However I know what you are thinking so I am open to changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. I can live with that, and it's still valid HTML, so I'm good. Will merge.
Description
as for now there was no way to set empty attribute like required or autofocus
Each pull request should address a single issue, and have a meaningful title.