44 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
Currently the 'Country' select field i not marked as mandatory so, browser side validation passes with this field not completed but, after submission the server side returns the user to the page and marks, Country, as required. This is not an ideal user experience. Proposed fix ------------- Add the required attribute to the Country select drop down.
Ok, so the reason why this happens is because as the docs state: "A Boolean attribute indicating that an option with a non-empty string value must be selected." As it will always at the very least be "-- select --" and never an empty string, it will pass default browser validation. This kinda sucks though, because the browser does warn you about all of the other required field but, when the user then submits the form, it comes back and says, whoops, you forgot one. I guess the chance of someone not selecting a country is rare but, it can happen so, should we add a little extra code to ensure the value of the select is not "-- select --" before passing it of to the server? Or should we leave it as is? There is also the option to add a line such as: "All fields are required unless marked optional" below the "Now tell us a little about yourself:" heading but, this add l10n to the mix unless we already have that string translated somewhere. Thoughts?
I would avoid adding new strings, and using existing strings to warn about the fact that this field is mandatory.
As far as I understand the value should not be "-- select --", it should be whatever is in the value property (in this case it's value="", which is falsy). Setting `required="required"` attribute on the select input seems to work. Sorry if I'm misunderstanding something else.
Hmm... "select" shouldn't be accepted as valid because that option has an empty value. Even though it has a text label, value="" should override it (the text is submitted in lieu of value if the value attribute is missing, but a blank value attribute trumps the option text). Does that option not have an empty value? If necessary we could just remove the "-- select --" text from the first option so it's just blank, and it wouldn't be adding any strings. I do like showing it as a hint, though, since we've styled selects to look just like text fields (and selects don't have a placeholder).
Thanks for the feedback all and yes, I would have thought the fact that the value attr is empty would cause validation to fail but, it seems to not be doing that. Let me experiment some more with this and see. It is probably something silly I am missing.
(In reply to Schalk Neethling [:espressive] from comment #5) > Thanks for the feedback all and yes, I would have thought the fact that the > value attr is empty would cause validation to fail but, it seems to not be > doing that. Let me experiment some more with this and see. > > It is probably something silly I am missing. Setting the required attribute seemed to work for me when testing locally, but I only did it very quickly.
Created attachment 8637092 [details] [review] Link to Github pull-request: https://github.com/mozilla/bedrock/pull/3147
Commits pushed to master at https://github.com/mozilla/bedrock https://github.com/mozilla/bedrock/commit/8cefd1226cf7109cfb0ad4a9967b03440718a89d Fix Bug 1185468, set country as required and style as other required fields when state is invalid https://github.com/mozilla/bedrock/commit/2d0020c504d7f61d0dbf5f521d895ebbf73a7123 Merge pull request #3147 from schalkneethling/bug1185468-ensure-country-field-is-marked-as-required Fix Bug 1185468, set country as required and style as other required fields when state is invalid