Closed
Bug 1098251
Opened 10 years ago
Closed 10 years ago
New Contact view insists on an email address being entered even if I only want to enter a Firefox OS phone number
Categories
(Hello (Loop) :: Client, defect)
Hello (Loop)
Client
Tracking
(firefox35 fixed, firefox36 fixed)
People
(Reporter: mreavy, Assigned: jaws)
References
Details
Attachments
(1 file, 4 obsolete files)
18.76 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
Bug 1081322 added the ability to enter a Firefox OS phone number. When I tried to enter a contact with just a Firefox OS phone, I could not save the entry unless I included an email address as well.
Since FxA allows for email addresses or Firefox OS phone numbers, having either one (email address or FxOS phone number) present should be sufficient for a user to save the entry.
Assignee | ||
Comment 1•10 years ago
|
||
The plan here is to visually indicate that both fields are required if the user tries to submit the form with only the Name field filled in.
However, we will accept the field with the name and either the phone or email address present.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Points: --- → 2
Updated•10 years ago
|
Flags: qe-verify?
Flags: firefox-backlog+
Assignee | ||
Comment 2•10 years ago
|
||
This actually turned out to be pretty simple since we were already using linkedState.
Attachment #8522503 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8522503 [details] [diff] [review]
Patch
Review of attachment 8522503 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/loop/content/js/contacts.jsx
@@ +575,5 @@
> switch (this.props.mode) {
> case "edit":
> this.state.contact.name[0] = this.state.name.trim();
> setPreferred(this.state.contact,
> + {field: "email", value: this.state.email.trim(), optional: true});
Both calls to setPreferred now use the optional:true argument. We could update setPreferred to always treat it as optional, but I'm on the fence. What do you think?
@@ +633,5 @@
> <input ref="name" required pattern="\s*\S.*" type="text"
> className={cx({pristine: this.state.pristine})}
> valueLink={this.linkState("name")} />
> <label>{mozL10n.get("edit_contact_email_label")}</label>
> + <input ref="email" type="email" required={phoneOrEmailRequired}
In case you're curious, if required=false, React won't create the "required" attribute.
Once a value appears in this field the "required" attribute is removed, but you can't submit a type="email" field with a non-email value, so it's OK that the field is not marked as "required" once a value is present.
linkState operates on the onChange event, so it is practically updated after each keystroke, meaning we don't have to worry about the requiredness-state of a field being out of date.
Comment 4•10 years ago
|
||
Comment on attachment 8522503 [details] [diff] [review]
Patch
Review of attachment 8522503 [details] [diff] [review]:
-----------------------------------------------------------------
Tests to make sure we handle the 4 cases (all combinations of phone and email fields {empty, valid}) would be good. We might already have tests for some of the 4.
Attachment #8522503 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8522503 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Carrying forward r+ from MattN.
NiKo`, do you know why the test is failing? It fails at line 99 and also at line 113.
Flags: needinfo?(nperriault)
Attachment #8522597 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8522573 -
Attachment is obsolete: true
Comment on attachment 8522597 [details] [diff] [review]
Patch with tests failing for unknown reasons
Review of attachment 8522597 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/loop/test/desktop-local/contacts_test.js
@@ +91,5 @@
> + loop.contacts.ContactDetailsForm({mode: "add"}));
> + var nameInput = view.getDOMNode().querySelector("input[type='text']");
> + nameInput.value = "Jenny";
> + var telInput = view.getDOMNode().querySelector("input[type='tel']");
> + telInput.value = "867-5309";
This is not the way you're supposed to update a field value; you should try using the TestUtils.Simulate event helpers: http://facebook.github.io/react/docs/test-utils.html#simulate
Try something like this instead:
TestUtils.Simulate.change(telInput, {target: {value: "867-5309"}});
Clearing needinfo.
Flags: needinfo?(nperriault)
Assignee | ||
Updated•10 years ago
|
Flags: qe-verify?
Flags: qe-verify-
Flags: in-testsuite+
Assignee | ||
Comment 9•10 years ago
|
||
Thanks Niko, that was exactly my problem. I had been trying to call the Simulate functionality but wasn't passing the new value to the Simulate function. With that change it all works as expected. I'll see if I can file a bug with Chai to get their documentation made clearer.
Attachment #8522597 -
Attachment is obsolete: true
Attachment #8523012 -
Flags: review+
Assignee | ||
Comment 10•10 years ago
|
||
s/Chai/React/
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8523012 -
Attachment is obsolete: true
Attachment #8523050 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Whiteboard: [fixed-in-fx-team]
Comment 12•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
Comment 13•10 years ago
|
||
Updated•10 years ago
|
status-firefox35:
--- → fixed
status-firefox36:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•