[de-xbl] rework daypicker binding (which inherits from button-base)
Categories
(Calendar :: General, task)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: aleca)
References
Details
Attachments
(1 file, 6 obsolete files)
20.16 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
daypicker inherits from button-base which will be removed in bug 1519577.
https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/calendar/base/content/calendar-daypicker.xml#16
<daypicker> is used only in he recurrence dialog (e.g. when repeat weekly chosen): https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/calendar/base/content/dialogs/calendar-event-dialog-recurrence.xul#140
Given it's very limited usage, not sure this needs to be a custom element. It could be clearer just doing it in code. Not sure what it does, looks like dispatching day selections to https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/calendar/resources/content/datetimepickers/datetimepickers.js#805 (?)
Reporter | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
I updated the daypicker
to be a custom element.
Since the element has a lot of attributes and uses a couple of hbox for the styling, I think it's easier to handle it as a custom element.
If you think this is a bit of an overkill, I can quickly convert it to a simple element.
Assignee | ||
Comment 2•4 years ago
|
||
Slightly updated code.
I'm seeing this error after a monthly daypicker is clicked.
It doesn't happen for the weekly daypicker and I can't figure out where this recursion is happening.
JavaScript error: resource://calendar/calendar-js/calRecurrenceInfo.js, line 480: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "Same occurrence found twice, protecting you from death by recursion" {file: "resource://calendar/modules/ical.js" line: 7193}]'[JavaScript Error: "Same occurrence found twice, protecting you from death by recursion" {file: "resource://calendar/modules/ical.js" line: 7193}]' when calling method: [calIRecurrenceRule::getOccurrences]
Assignee | ||
Comment 3•4 years ago
|
||
Rolling back to the previous patch as I introduced some bugs while trying to clean it up.
Sorry about that.
Assignee | ||
Updated•4 years ago
|
Reporter | ||
Comment 4•4 years ago
|
||
Comment on attachment 9066887 [details] [diff] [review] dexbl-daypicker.patch Review of attachment 9066887 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/calendar-day-picker.js @@ +3,5 @@ > +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/* global MozXULElement, setBooleanAttribute, getSummarizedStyleValues, cal */ > + > +class MozCalendarDayPicker extends customElements.get("button") { please add documentation too and @extends MozButton @@ +6,5 @@ > + > +class MozCalendarDayPicker extends customElements.get("button") { > + static get observedAttributes() { > + return ["label"]; > + } instead of using observedAttributes attributeChangedCallback and the related helpers you should be able to only have static get inheritedAttributes() { return { ".toolbarbutton-text" : "label=value" } } @@ +9,5 @@ > + return ["label"]; > + } > + > + connectedCallback() { > + if (this.delayConnectedCallback()) { also need to check if (this.hasChildNodes()) ... to prevent adding more when connectedCallback() runs more than once @@ +14,5 @@ > + return; > + } > + > + const hbox = document.createElement("hbox"); > + hbox.classList.add("daypickerclass"); daypickerclass is never used so we can remove it @@ +18,5 @@ > + hbox.classList.add("daypickerclass"); > + hbox.setAttribute("flex", "1"); > + hbox.setAttribute("align", "center"); > + > + const label = document.createElement("label"); createXULElement (we're making sure the namespace is correct now) @@ +66,5 @@ > + } > + } > +} > + > +customElements.define("daypicker", MozCalendarDayPicker); normally CE must have a dash in their name, so let's fix that here now (there are also bugs for the internal hack that they don't, like you can't inherit from them) So change to calendar-daypicker and MozCalendarDaypicker
Assignee | ||
Comment 5•4 years ago
|
||
Thanks for the quick review.
I updated everything and also fixed that console error I was getting, caused by the use of const
variables.
Reporter | ||
Comment 6•4 years ago
|
||
Comment on attachment 9066961 [details] [diff] [review] dexbl-daypicker.patch Review of attachment 9066961 [details] [diff] [review]: ----------------------------------------------------------------- Something's wrong, the "buttons" are all empty, no text showing. Looking more closely, I'm not sure you should be adding the label and all that as children. This is now inheriting a button, and that already has what's needed internally, no? For the file name: calendar-daypicker.js without the extra hypen Should still warp the implementation in // Wrap in a block to prevent leaking to window scope. { ::: calendar/base/content/calendar-day-picker.js @@ +3,5 @@ > +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/** > + * The MozCalendarDaypicker widget is used to display the days of > + * the week and month for the reminder repeater dialog. ... and month for event recurrence. @@ +7,5 @@ > + * the week and month for the reminder repeater dialog. > + * > + * @extends {MozButton} > + */ > + nit: remove the empty line
Assignee | ||
Comment 7•4 years ago
|
||
I needed to call super.connectedCallback();
in order for the button to behave as expected.
The label should show up now.
Should I add this attributes when I'm defining the custom element? { extends: "button" }
I thought I had to but as soon as I add it the button goes completely blank.
Reporter | ||
Comment 8•4 years ago
|
||
You can only inherit for the right element (daypicker needs to be a <button is="calendar-caypicker">, not <calendar-daypicker)
Assignee | ||
Comment 9•4 years ago
|
||
Updated accordingly.
Reporter | ||
Comment 10•4 years ago
|
||
Looks good. I just fixed a few style nits
Updated•4 years ago
|
Comment 11•4 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/4ae242ef0d4c
[de-xbl] convert the daypicker binding to <button is="calendar-daypicker">. r=mkmelin
Updated•4 years ago
|
Description
•