-
Notifications
You must be signed in to change notification settings - Fork 619
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
Add support for standard-compliant placeholder option #617
Conversation
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.
Would you mind incrementing a minor version please? Thanks!
return ( | ||
this.element.querySelector('option[value=""]') || | ||
this.element.querySelector('option[placeholder]') | ||
); |
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.
Would you mind just adding a comment here explaining why both are supported please 👍
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.
@jshjohnson done.
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.
Would you also mind incrementing a minor version please (not major - ignore my original comment )
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.
done (as a separate commit, as updating the build file for the new version also included other changes which were not yet in it).
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.
@jshjohnson @stof That should be done in the same .querySelector('option[value=""], option[placeholder]')
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.
@tinovyatkin no, that's not the same. Your proposal would use whatever is found first. My code is using the standard way if it is there (and so if you define both, the standard way wins)
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.
@jshjohnson
Ugh, if somebody defines both, one with invalid, but very implicit and documented (by this library) attribute placeholder
and second one just with empty value and empty value starts winning - I think it's a breaking change.
d6dbecf
to
6e5d324
Compare
* Add support for standard-compliant placeholder option * Bump version and rebuild files
Description
Fixes #447
Replaces #474. The implementation is almost the same. But my PR preserves backward compatibility by using
<option placeholder>
as a fallback when there is no<option value="">
.Types of changes
Checklist: