Closed Bug 1422164 Opened 7 years ago Closed 7 years ago

Create a rich-select Custom Element

Categories

(Firefox :: WebPayments UI, enhancement, P1)

Product:
Firefox
Component:
WebPayments UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Depends on 1 open bug)

Details

(Whiteboard: [webpayments])

User Story

Markup TBD

Attachments

(1 file)

Bug 1422164 - Create a rich-select, address-option, and basic-card-option custom elements.
59 bytes, text/x-review-board-request
MattN
: review+
Details
No description provided.
Priority: -- → P1
Blocks: 1427950
Comment on attachment 8937754 [details] Bug 1422164 - Create a rich-select, address-option, and basic-card-option custom elements. https://reviewboard.mozilla.org/r/208466/#review217004 Thanks ::: commit-message-55724:1 (Diff revision 2) > +Bug 1422164 - WIP, create a rich-select Custom Element. Remove WIP from the commit message ::: toolkit/components/payments/res/components/rich-select.js:24 (Diff revision 2) > + > +/* global ObservedPropertiesMixin */ > + > +class RichSelect extends ObservedPropertiesMixin(HTMLElement) { > + static get observedAttributes() { > + return ["open", "disabled", "hidden"]; Nit: one per line with a trailing comma would be better for tracking blame. Apologies if I didn't follow this myself. I thought we actually had an eslint rule for that for the case where the array contains more than one value. ::: toolkit/components/payments/res/components/rich-select.js:76 (Diff revision 2) > + if (next) { > + next.selected = true; > + selectedChild.selected = false; > + } We'll eventually need to distinguish the option that is selected from the option that the user is highlighting while the dropdown is open. At least on macOS I think the selectedness doesn't change until the user hits enter on the highlighted row. Consider: We wouldn't want to tell the merchant that a new shipping address was chosen each time the arrow key was pressed until the popup closed on the dropdown. We can deal with that when we hook this up to the state object. ::: toolkit/components/payments/res/components/rich-select.js:192 (Diff revision 2) > + fragment.appendChild(element); > + return element; > + } > +} > + > +customElements.define("rich-option", RichOption); You don't need to register this element if it's only being used for inheritance. ::: toolkit/components/payments/res/components/rich-select.js:194 (Diff revision 2) > + } > +} > + > +customElements.define("rich-option", RichOption); > + > +class AddressOption extends ObservedPropertiesMixin(RichOption) { Please move `AddressOption` and `BasicCardOption` each to their own files. ::: toolkit/components/payments/res/components/rich-select.js:212 (Diff revision 2) > + constructor() { > + super(); > + } IIRC the default constructor already does this so this can be omitted. ::: toolkit/components/payments/res/components/rich-select.js:217 (Diff revision 2) > + for (let child of this.children) { > + child.remove(); > + } > + > + let fragment = document.createDocumentFragment(); It would be more performant if both of the Option classes only created the elements in the `constructor` or `connectedCallback` and assigned the elements references to a private variable and then have render just set `element.textContent` on the references. That would mean that a render would only affect the `textContent` and wouldn't have the overhead of document fragments and creating elements and setting classes. For example, see line 78 of https://reviewboard.mozilla.org/r/196014/diff/4#file5723914 It's not strictly necessary at this point since we won't re-render often yet but it may be good to get into the practice of reducing overhead. You can file this as a follow-up if you'd like. ::: toolkit/components/payments/res/components/rich-select.js:245 (Diff revision 2) > + constructor() { > + super(); > + } IIRC the default constructor already does this so this can be omitted. ::: toolkit/components/payments/res/paymentRequest.xhtml:12 (Diff revision 2) > <head> > <meta http-equiv="Content-Security-Policy" content="default-src 'self'"/> > <title></title> > <link rel="stylesheet" href="paymentRequest.css"/> > + <link rel="stylesheet" href="rich-select.css"/> > - <script src="vendor/custom-elements.min.js"></script> > + <script src="vendor/custom-elements.min.js"></script> I think there is accidental indentation here ::: toolkit/components/payments/res/paymentRequest.xhtml:42 (Diff revision 2) > </head> > <body> > <iframe id="debugging-console" hidden="hidden" src="debugging.html"></iframe> > > + <rich-select> > + <address-option email="emzembrano92@email.com" Nit: please don't use real third-party domains for examples, use RFC2606 TLDs or second level domain names: https://tools.ietf.org/html/rfc2606#section-2 ::: toolkit/components/payments/test/mochitest/mochitest.ini:5 (Diff revision 2) > [DEFAULT] > support-files = > ../../../../../testing/modules/sinon-2.3.2.js > ../../res/PaymentsStore.js > + ../../res/rich-select.css I think the stylesheet should stay beside the component JS file in the same directory. ::: toolkit/components/payments/test/mochitest/test_rich_select.html:66 (Diff revision 2) > +function is_visible(element, message) { > + let rect = element.getBoundingClientRect(); > + return !rect.width && !rect.height; > +} > + > +function is_hidden(element, message) { > + return !is_visible(element, message); > +} You can use `isHidden` from EventUtils.js instead: https://dxr.mozilla.org/mozilla-central/rev/ca379fcca95b1f4a3744242ea8647004b99b3507/testing/mochitest/tests/SimpleTest/EventUtils.js#158 Add ```html <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script> ``` after the SpawnTask.js line. ::: toolkit/components/payments/test/mochitest/test_rich_select.html:170 (Diff revision 2) > + > + dispatchKeyPress("ArrowUp", 38); > + ok(option1.selected, "option 1 should remain selected"); > + ok(!option3.selected, "option 3 should not be selected"); > + > + // Wait for any mutation observer notifications to come apply before exiting. Nit: "to come apply" is confusing to read
Attachment #8937754 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8937754 [details] Bug 1422164 - Create a rich-select, address-option, and basic-card-option custom elements. https://reviewboard.mozilla.org/r/208466/#review217004 > Remove WIP from the commit message It looks like somehow my last revision never got published. I've just published it now (where this is fixed) and the behavior of opening the select is better in the latest revision. Fixing the other issues now.
Blocks: 1429195
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/90da1b24e686 Create a rich-select, address-option, and basic-card-option custom elements. r=MattN
Flags: needinfo?(jaws)
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ee57508c6e70 Create a rich-select, address-option, and basic-card-option custom elements. r=MattN
https://hg.mozilla.org/mozilla-central/rev/ee57508c6e70
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1431368
Whiteboard: [webpayments]
Depends on: 1446577
Product: Toolkit → Firefox
Target Milestone: mozilla59 → Firefox 59
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: