Closed
Bug 1440499
Opened 6 years ago
Closed 6 years ago
Implement the payerName/payerEmail/payerPhone contact picker
Categories
(Firefox :: WebPayments UI, enhancement, P1)
Firefox
WebPayments UI
Tracking
()
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: MattN, Assigned: sfoster)
References
()
Details
(Whiteboard: [webpayments])
Attachments
(3 files, 2 obsolete files)
We need to handle requestPayerName/requestPayerEmail/requestPayerPhone by providing UI to pick a profile for these. We can re-use our address-picker but we should only show the relevant fields.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8954496 [details] Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker https://reviewboard.mozilla.org/r/223564/#review229672 Here are comments of things to fix on my WIP patch. ::: toolkit/components/payments/res/components/address-option.css:13 (Diff revision 1) > rich-select[open] > .rich-select-popup-box > address-option { > grid-template-areas: > "name name " > "street-address street-address" > "email tel "; I think `email` shouldn't be in the default layout since it's only part of the payer/contact picker, not shipping or billing address. ::: toolkit/components/payments/res/components/address-option.css:20 (Diff revision 1) > +address-option[hide-address] { > + grid-template-areas: > + "name name" > + "tel email"; > +} The @hide-address approach was a hacky temporary method for a demo but I don't think it's scalable to handle the 4 boolean: * requestShipping * requestPayerName * requestPayerEmail * requestPayerPhone Jared mentioned maybe we shouldn't use `grid-template-areas`. I wondered if we can have an optional @fields attribute with space-separated field categories (e.g. name, address, email, phone) that affect the visibility of each type of data. We can maybe use the `E[field~="email"]` selector[1]. [1] https://www.w3.org/TR/selectors-3/#attribute-selectors ::: toolkit/components/payments/res/containers/payment-dialog.js:215 (Diff revision 1) > + let payerRequested = paymentOptions.requestPayerName || paymentOptions.requestPayerEmail || paymentOptions.requestPayerPhone; > + for (let element of this._payerRelatedEls) { > + element.hidden = !payerRequested; > + } This would need to be smarter and set separate attribute values on the picker so that we only show the requested types in the picker e.g. don't show the payer name if requestPayerName is false but requestPayerEmail is true.
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8954496 [details] Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker https://reviewboard.mozilla.org/r/223564/#review229678 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/payments/res/containers/payment-dialog.js:215 (Diff revision 1) > this._errorText.textContent = paymentDetails.error; > let paymentOptions = request.paymentOptions; > for (let element of this._shippingRelatedEls) { > element.hidden = !paymentOptions.requestShipping; > } > + let payerRequested = paymentOptions.requestPayerName || paymentOptions.requestPayerEmail || paymentOptions.requestPayerPhone; Error: Line 215 exceeds the maximum line length of 100. [eslint: max-len]
Reporter | ||
Updated•6 years ago
|
Assignee: MattN+bmo → sfoster
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
Matt: This is far enough along for an initial review. I've not squashed the commits - I hope that's not too confusing. So its your initial patch, a patch to add debugging options, then the functional changes. This seems to do what we want so its good checkpoint. I've not polished on the CSS - I can do more here, or leave this for another bug. Likewise I've not worked on the tests yet, but my intention is to do a couple simple mochitests that assert the expected elements show up un-hidden based on the paymentOption values we pass in.
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8955344 [details] Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker. https://reviewboard.mozilla.org/r/224516/#review230462 Code analysis found 2 defects in this patch: - 2 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/payments/res/containers/payment-dialog.js:165 (Diff revision 1) > + this.requestStore.setState({ > + selectedPayerAddress: Object.keys(savedAddresses)[0] || null, > + }); > + } > + > } Error: Block must not be padded by blank lines. [eslint: padded-blocks] ::: toolkit/components/payments/res/containers/payment-dialog.js:215 (Diff revision 1) > this._errorText.textContent = paymentDetails.error; > let paymentOptions = request.paymentOptions; > for (let element of this._shippingRelatedEls) { > element.hidden = !paymentOptions.requestShipping; > } > + let payerRequested = paymentOptions.requestPayerName || paymentOptions.requestPayerEmail || paymentOptions.requestPayerPhone; Error: Line 215 exceeds the maximum line length of 100. [eslint: max-len]
Reporter | ||
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8955345 [details] Bug 1440499 - Add debug options for the payer details and shipping requested. https://reviewboard.mozilla.org/r/224518/#review230564 ::: toolkit/components/payments/res/debugging.js:308 (Diff revision 1) > + let req = Object.assign(requestStore.getState().request, { > + paymentOptions: options, > + }); This is mutating the original request but it should be treated as immutable. You should clone the request first (with .assign on {}) and the add the paymentOptions to the clone.
Attachment #8955345 -
Flags: review?(MattN+bmo) → review+
Reporter | ||
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8955346 [details] Bug 1440499 - Add payerName/Email/Phone contact picker. https://reviewboard.mozilla.org/r/224520/#review230572 My original thinking was to not teach address-option about the "payer" concept in order to keep it as a generic "address option" so that's why I was saying there are 4 different kinds of fields (counting address as the fourth). Teaching it about the payer concept definitely makes implementation easier but I worry that it will be harder to maintain because we're introducing another mode. Please consider whether there is a way to avoid teaching address-option about the payer concept e.g. including "address" in the fieldNames.
Attachment #8955346 -
Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8955346 -
Attachment is obsolete: true
Assignee | ||
Comment 16•6 years ago
|
||
After saying that I could keep Matt's changes and mine in separate patches, when I checked again it would not leave patches that could really stand on their own. So I've folded those two together. I'm not sure how best to manager authorship here, particularly as it ends up with the author of part of the patch also being the reviewer. Let me know if I can change anything there.
Assignee | ||
Updated•6 years ago
|
Attachment #8954496 -
Attachment is obsolete: true
Reporter | ||
Comment 17•6 years ago
|
||
mozreview-review |
Comment on attachment 8955345 [details] Bug 1440499 - Add debug options for the payer details and shipping requested. https://reviewboard.mozilla.org/r/224518/#review231540 ::: toolkit/components/payments/res/debugging.html:23 (Diff revision 3) > + <label><input type="checkbox" autocomplete="off" name="requestPayerName" id="setRequestPayerName">requestPayerName</label> > + <label><input type="checkbox" autocomplete="off" name="requestPayerEmail" id="setRequestPayerEmail">requestPayerEmail</label> > + <label><input type="checkbox" autocomplete="off" name="requestPayerPhone" id="setRequestPayerPhone">requestPayerPhone</label> Please add requestShipping as a checkbox and then we can probably change/remove the "Contact & No Shipping" button. As-is, if you check any of the three, requestShipping will be removed which is frustrating and confusing.
Reporter | ||
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8955344 [details] Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker. https://reviewboard.mozilla.org/r/224516/#review231542 ::: toolkit/components/payments/res/components/address-option.css:42 (Diff revision 3) > +/* Hide all the fields by default, and enable them explicitly for each format case */ > address-option > .name, > address-option > .street-address, > address-option > .email, > address-option > .tel { > white-space: nowrap; > + display: none; > } > > -.rich-select-selected-clone > .email, > -.rich-select-selected-clone > .tel { > - display: none; > +address-picker:not([address-fields]) address-option > .name, > +address-picker:not([address-fields]) address-option > .street-address { > + display: inline-block; > +} This commit regresses the behaviour to show the telephone number in the dropdown when open but not in the selected option when closed and I believe that is still in the UX/UI spec: https://mozilla.invisionapp.com/share/SDFY4PA4EQ7#/screens/280226161
Attachment #8955344 -
Flags: review?(MattN+bmo)
Comment 19•6 years ago
|
||
(In reply to Sam Foster [:sfoster] from comment #16) > After saying that I could keep Matt's changes and mine in separate patches, > when I checked again it would not leave patches that could really stand on > their own. So I've folded those two together. I'm not sure how best to > manager authorship here, particularly as it ends up with the author of part > of the patch also being the reviewer. Let me know if I can change anything > there. In the past I've updated the commit message to include "Original patch authored by MattN" for example, thus to give credit where it's due. It is OK to have MattN still be the reviewer, as it is assumed that you, the new author, implicitly reviewed his changes and agreed with them (where they were left untouched or at least not drastically changed).
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8955344 [details] Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker. https://reviewboard.mozilla.org/r/224516/#review231542 > This commit regresses the behaviour to show the telephone number in the dropdown when open but not in the selected option when closed and I believe that is still in the UX/UI spec: https://mozilla.invisionapp.com/share/SDFY4PA4EQ7#/screens/280226161 It sounds like this may still need final definition from UX. For now I think the new changes should restore the behavior where we show name, email, tel in the dropdown options, but only display the requested fields in the selected option when the dropdown is closed.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8955344 [details] Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker. https://reviewboard.mozilla.org/r/224516/#review231884 ::: toolkit/components/payments/res/components/address-option.css:21 (Diff revisions 5 - 6) > grid-template-areas: > - "name name" > - "tel email"; > + "name" > + "email" > + "tel"; > } Why did you change this now? It matches https://mozilla.invisionapp.com/share/YAFI31XR3KP#/screens/275361869 ::: toolkit/components/payments/res/components/address-option.css:51 (Diff revisions 5 - 6) > -address-picker.shipping-related .rich-select-selected-clone > .tel { > +address-picker.shipping-related address-option > .tel { > display: none; > } You only needed to change the email line, the `tel` line should go back to the previous version. The telephone number should show on the shipping picker when it's open.
Attachment #8955344 -
Flags: review?(MattN+bmo)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8955344 [details] Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker. https://reviewboard.mozilla.org/r/224516/#review232210 Thanks! Sorry about the back and forth
Attachment #8955344 -
Flags: review?(MattN+bmo) → review+
Comment 28•6 years ago
|
||
Pushed by sfoster@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/479b143d8828 Add debug options for the payer details and shipping requested. r=MattN https://hg.mozilla.org/integration/autoland/rev/4e2e081dad55 Implement the payerName/payerEmail/payerPhone contact picker. r=MattN
Comment 29•6 years ago
|
||
Backed out for ESlint failure on paymentDialogWrapper.js:53 backout: https://hg.mozilla.org/integration/autoland/rev/5fb1b1d9d6699eacb2d749edc095d0f5b2d3fe4f push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4e2e081dad557a8c060a3751bb1986c9686d5fe2&selectedJob=166878841 failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=166878841&repo=autoland&lineNumber=254 [task 2018-03-09T00:31:43.805Z] running build_ext [task 2018-03-09T00:31:43.805Z] building 'psutil._psutil_linux' extension [task 2018-03-09T00:31:43.805Z] creating build [task 2018-03-09T00:31:43.805Z] creating build/temp.linux-x86_64-2.7 [task 2018-03-09T00:31:43.805Z] creating build/temp.linux-x86_64-2.7/psutil [task 2018-03-09T00:31:43.805Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o [task 2018-03-09T00:31:43.806Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o [task 2018-03-09T00:31:43.806Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o [task 2018-03-09T00:31:43.806Z] creating build/lib.linux-x86_64-2.7 [task 2018-03-09T00:31:43.806Z] creating build/lib.linux-x86_64-2.7/psutil [task 2018-03-09T00:31:43.807Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so [task 2018-03-09T00:31:43.807Z] building 'psutil._psutil_posix' extension [task 2018-03-09T00:31:43.807Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o [task 2018-03-09T00:31:43.808Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o [task 2018-03-09T00:31:43.808Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so [task 2018-03-09T00:31:43.808Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil [task 2018-03-09T00:31:43.808Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil [task 2018-03-09T00:31:43.808Z] [task 2018-03-09T00:31:43.808Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt) [task 2018-03-09T00:40:18.217Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/payments/content/paymentDialogWrapper.js:53:23 | 'profileStorage' is not defined. (no-undef) [taskcluster 2018-03-09 00:40:19.102Z] === Task Finished === [taskcluster 2018-03-09 00:40:19.103Z] Unsuccessful task run with exit code: 1 completed in 738.268 seconds
Flags: needinfo?(sfoster)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 32•6 years ago
|
||
Thanks for the back-out. The patches needed to be rebased on a more up-to-date central to pick up a change that renamed profileStorage to formAutofillStorage.
Flags: needinfo?(sfoster)
Comment 33•6 years ago
|
||
Pushed by sfoster@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d1a65f74a21c Add debug options for the payer details and shipping requested. r=MattN https://hg.mozilla.org/integration/autoland/rev/f1965cf7425f Implement the payerName/payerEmail/payerPhone contact picker. r=MattN
Comment 34•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d1a65f74a21c https://hg.mozilla.org/mozilla-central/rev/f1965cf7425f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•6 years ago
|
Whiteboard: [webpayments]
Updated•6 years ago
|
Product: Toolkit → Firefox
Target Milestone: mozilla60 → Firefox 60
You need to log in
before you can comment on or make changes to this bug.
Description
•