Closed
Bug 1512942
Opened 6 years ago
Closed 6 years ago
[de-xbl] Convert timepicker-minute to custom element.
Categories
(Calendar :: Lightning Only, enhancement)
Calendar
Lightning Only
Tracking
(Not tracked)
RESOLVED
FIXED
6.9
People
(Reporter: arshad, Assigned: arshad)
References
Details
Attachments
(1 file, 6 obsolete files)
|
(deleted),
patch
|
arshad
:
review+
arshad
:
feedback+
|
Details | Diff | Splinter Review |
No description provided.
|
Assignee | |
Updated•6 years ago
|
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED
|
|
Assignee | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 1•6 years ago
|
||
Attachment #9030416 -
Flags: feedback?(mkmelin+mozilla)
|
||
Comment 2•6 years ago
|
||
Comment on attachment 9030416 [details] [diff] [review]
timepicker-minute.patch
Review of attachment 9030416 [details] [diff] [review]:
-----------------------------------------------------------------
::: common/bindings/datetimepicker.js
@@ +83,5 @@
> + return;
> + }
> +
> + if (this.hasAttribute("label")) {
> + this.label.setAttribute("value", this.getAttribute("label"));
should handle label removal too
::: mail/base/content/customElements.js
@@ +10,5 @@
> "chrome://messenger/content/mailWidgets.js",
> "chrome://messenger/content/generalBindings.js",
> "chrome://messenger/content/statuspanel.js",
> "chrome://messenger/content/foldersummary.js",
> + "chrome://messenger/content/datetimepicker.js",
-> lightning
::: mail/base/jar.mn
@@ +38,5 @@
> * content/messenger/viewSource.xul (../../common/src/viewSource.xul)
> * content/messenger/datetimepicker.xml (../../common/bindings/datetimepicker.xml)
> content/messenger/generalBindings.xml (../../common/bindings/generalBindings.xml)
> content/messenger/generalBindings.js (../../common/bindings/generalBindings.js)
> + content/messenger/datetimepicker.js (../../common/bindings/datetimepicker.js)
-> lightning
Attachment #9030416 -
Flags: feedback?(mkmelin+mozilla) → feedback-
|
|
Assignee | |
Comment 3•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #2)
> Comment on attachment 9030416 [details] [diff] [review]
> timepicker-minute.patch
>
> Review of attachment 9030416 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: common/bindings/datetimepicker.js
> @@ +83,5 @@
> > + return;
> > + }
> > +
> > + if (this.hasAttribute("label")) {
> > + this.label.setAttribute("value", this.getAttribute("label"));
>
> should handle label removal too
>
I know we have done this earlier, but recently I was workin on a bug where I landed it some problems when I removed the attr on inheriting elment when parent element's attr was removed.
|
|
||
Comment 4•6 years ago
|
||
That's something to watch out for. Dunno if it happens in reality for this case, but if someone does remove the label you'd have unexpected behaviour unless you handle it.
|
|
Assignee | |
Comment 5•6 years ago
|
||
>
> ::: mail/base/jar.mn
> @@ +38,5 @@
> > * content/messenger/viewSource.xul (../../common/src/viewSource.xul)
> > * content/messenger/datetimepicker.xml (../../common/bindings/datetimepicker.xml)
> > content/messenger/generalBindings.xml (../../common/bindings/generalBindings.xml)
> > content/messenger/generalBindings.js (../../common/bindings/generalBindings.js)
> > + content/messenger/datetimepicker.js (../../common/bindings/datetimepicker.js)
>
>
why is this in common/bindings, if they were lightning specific only.
|
|
||
Comment 6•6 years ago
|
||
It's not, that's from one of your other unlanded patches.
|
|
Assignee | |
Comment 7•6 years ago
|
||
Attachment #9030416 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 8•6 years ago
|
||
Attachment #9030697 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 9•6 years ago
|
||
Comment on attachment 9030699 [details] [diff] [review]
timepicker-minute.patch
Review of attachment 9030699 [details] [diff] [review]:
-----------------------------------------------------------------
::: calendar/base/content/dialogs/calendar-event-dialog-attendees.xul
@@ +29,5 @@
> style="padding-top: 8px; padding-bottom: 10px; padding-inline-start: 8px; padding-inline-end: 10px;"
> xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>
> <!-- Javascript includes -->
> + <script type="application/javascript"
Adding script in files where datetimepicker is used because datetimepicker has timepicker as anon content, which has timpicker-grids inside it.. https://searchfox.org/comm-central/search?q=datetimepicker&case=false®exp=false&path=calendar
Attachment #9030699 -
Flags: feedback?(mkmelin+mozilla)
|
|
||
Comment 10•6 years ago
|
||
Comment on attachment 9030699 [details] [diff] [review]
timepicker-minute.patch
Review of attachment 9030699 [details] [diff] [review]:
-----------------------------------------------------------------
Doesn't seem to work. Clicking a minute I get
JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.js, line 62: ReferenceError: clickMinute is not defined
::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +5,5 @@
> +"use strict";
> +
> +// This is loaded into all XUL windows. Wrap in a block to prevent
> +// leaking to window scope.
> +{
I think you should skip the indent for this though
It's also questionable if we really care. It's not all XUL windows, it's just the ones we use it in.
Attachment #9030699 -
Flags: feedback?(mkmelin+mozilla) → feedback-
|
|
Assignee | |
Comment 11•6 years ago
|
||
See Bug 1512941 for the timepicker-minute patch changes.
|
|
Assignee | |
Comment 12•6 years ago
|
||
Attachment #9030699 -
Attachment is obsolete: true
Attachment #9047947 -
Flags: review+
|
|
Assignee | |
Comment 13•6 years ago
|
||
|
|
||
Comment 14•6 years ago
|
||
Comment on attachment 9047947 [details] [diff] [review]
timepicker-minute.patch
Review of attachment 9047947 [details] [diff] [review]:
-----------------------------------------------------------------
(no reviewed yet!)
::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +71,5 @@
> + this.appendChild(spacer.cloneNode());
> + this.appendChild(minutebox);
> + this.appendChild(spacer);
> +
> + this.pixelScrollDelta = 0;
why does all this dom building happen in connecedCallback() and not the constructor?
Attachment #9047947 -
Flags: review+
|
|
Assignee | |
Comment 15•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #14)
Comment on attachment 9047947 [details] [diff] [review]
timepicker-minute.patchReview of attachment 9047947 [details] [diff] [review]:
(no reviewed yet!)
::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +71,5 @@
this.appendChild(spacer.cloneNode());this.appendChild(minutebox);this.appendChild(spacer);this.pixelScrollDelta = 0;why does all this dom building happen in connecedCallback() and not the
constructor?
if it is not shadow dom then dom should be build in connectedCallback. It will throw NSERrror if you append child in constructor and try to extend it by some other ce. We experienced this in one of the very first ce conversion.
|
|
Assignee | |
Comment 16•6 years ago
|
||
Comment on attachment 9047947 [details] [diff] [review]
timepicker-minute.patchReview of attachment 9047947 [details] [diff] [review]:
(no reviewed yet!)
::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +71,5 @@
this.appendChild(spacer.cloneNode());this.appendChild(minutebox);this.appendChild(spacer);this.pixelScrollDelta = 0;
Fallen reviewed it in https://bugzilla.mozilla.org/show_bug.cgi?id=1512941#c24(In reply to Magnus Melin [:mkmelin] from comment #14)
|
|
||
Comment 17•6 years ago
|
||
You really need to put that kind of information into this bug when marking a patch r+
I'm not convinced you should build in connectedCallback. That can run more than once.
|
|
Assignee | |
Comment 18•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #17)
You really need to put that kind of information into this bug when marking a patch r+
Yes you are right, I should have put it here also. I forgot about it after conveying it to you on IRC.
I'm not convinced you should build in connectedCallback. That can run more than once.
when you clone node, or move nodes..
|
|
Assignee | |
Comment 19•6 years ago
|
||
is it ok now?
Attachment #9047947 -
Attachment is obsolete: true
Attachment #9047958 -
Flags: review+
Attachment #9047958 -
Flags: feedback?(mkmelin+mozilla)
|
|
Assignee | |
Comment 20•6 years ago
|
||
Attachment #9047958 -
Attachment is obsolete: true
Attachment #9047958 -
Flags: feedback?(mkmelin+mozilla)
Attachment #9047989 -
Flags: review+
Attachment #9047989 -
Flags: feedback?(mkmelin+mozilla)
|
|
||
Comment 21•6 years ago
|
||
Comment on attachment 9047989 [details] [diff] [review]
timepicker-minute.patch
Review of attachment 9047989 [details] [diff] [review]:
-----------------------------------------------------------------
::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +602,5 @@
> if ((preString && this.amRegExp.test(preString)) ||
> (postString && this.amRegExp.test(postString))) {
> ampmCode = "AM";
> } else if ((preString && this.pmRegExp.test(preString)) ||
> + (postString && this.pmRegExp.test(postString))) {
strange intention change here
@@ +761,5 @@
> this.ampmIndex = POST_INDEX;
> }
> }
> if (this.ampmIndex) {
> + let makeFormatRegExp = function (string) {
no space after function (leave this line as it was)
Attachment #9047989 -
Flags: feedback?(mkmelin+mozilla) → feedback+
|
|
Assignee | |
Comment 22•6 years ago
|
||
|
|
Assignee | |
Comment 23•6 years ago
|
||
Attachment #9047989 -
Attachment is obsolete: true
Attachment #9048056 -
Flags: review+
Attachment #9048056 -
Flags: feedback+
|
|
Assignee | |
Updated•6 years ago
|
Keywords: checkin-needed
|
|
Assignee | |
Comment 24•6 years ago
|
||
|
|
||
Comment 25•6 years ago
|
||
Just a note that bugzilla autolinks bug comments when you write something like bug 1512942 comment 22
|
||
Comment 26•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9385db6fc2ae
Convert timepicker-minute binding to custom element. r=philipp
|
||
Updated•6 years ago
|
Target Milestone: --- → 6.9
You need to log in
before you can comment on or make changes to this bug.
Description
•