Closed
Bug 1512945
Opened 6 years ago
Closed 6 years ago
[de-xbl] Migrate calendar-day-label to custom element.
Categories
(Calendar :: Lightning Only, enhancement)
Calendar
Lightning Only
Tracking
(Not tracked)
RESOLVED
FIXED
6.8
People
(Reporter: arshad, Assigned: arshad)
References
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
arshad
:
review+
arshad
:
feedback+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #9030205 -
Flags: review?(makemyday)
Attachment #9030205 -
Flags: feedback?(mkmelin+mozilla)
Comment 2•6 years ago
|
||
Comment on attachment 9030205 [details] [diff] [review] calendar-day-label.patch Review of attachment 9030205 [details] [diff] [review]: ----------------------------------------------------------------- The days shrink and do not take up the full width like they used to. Adding this.setAttribute("flex", "1"); ... in connectedCallback seems to help with that ::: calendar/base/content/calendar-base-view.js @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/* global MozXULElement */ probably global setBooleanAttribute too @@ +109,5 @@ > + } else { > + // in this case the weekdaypixels have not yet been layouted; > + // by definition 0 is returned > + return 0; > + } could take the opportunity and do this, instead of thee if-else if (longNameWidth == 0) { // weekdaypixels have not yet been layed out return 0; } ::: 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://calendar/content/calendar-base-view.js", as this is lightning functionality, load it for lightning (as a script file)
Attachment #9030205 -
Flags: review?(makemyday)
Attachment #9030205 -
Flags: feedback?(mkmelin+mozilla)
Attachment #9030205 -
Flags: feedback-
Assignee | ||
Comment 3•6 years ago
|
||
> as this is lightning functionality, load it for lightning (as a script file)
which file should I exactly put it in.. This binding will be used by many other bindings, is there a file which is must to get loaded for calendar?
Flags: needinfo?(mkmelin+mozilla)
Comment 4•6 years ago
|
||
In the corresponding file where the binding got loaded earlier, so in this case I guess https://searchfox.org/comm-central/search?q=calendar-view-bindings.css&path= ... on top of messenger-overlay-sidebar.xul
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #9030205 -
Attachment is obsolete: true
Attachment #9030655 -
Flags: review?(philipp)
Attachment #9030655 -
Flags: feedback?(mkmelin+mozilla)
Comment 6•6 years ago
|
||
Comment on attachment 9030655 [details] [diff] [review] calendar-day-label.patch Review of attachment 9030655 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/calendar-base-view.js @@ +24,5 @@ > + > + this.appendChild(this.longWeekdayName); > + this.appendChild(this.shortWeekdayName); > + > + this.setAttribute("flex", "1"); would move this up, perhaps before this.textContent @@ +108,5 @@ > + return 0; > + } > + > + this.longWeekdayPixels = longNameWidth + > + getSummarizedStyleValues(this.longName, ["margin-left", "margin-right"]); did you check eslint? getSummarizedStyleValues also appeears to be a global ::: calendar/lightning/content/messenger-overlay-sidebar.xul @@ +31,5 @@ > <?xul-overlay href="chrome://lightning/content/lightning-menus.xul"?> > > <overlay id="ltnSidebarOverlay" > xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> > + <!-- NEEDED FOR SOME CUSTOM ELEMENTS USED IN LIGHTNING --> don't think the comment is needed
Assignee | ||
Comment 7•6 years ago
|
||
Attachment #9030655 -
Attachment is obsolete: true
Attachment #9030655 -
Flags: review?(philipp)
Attachment #9030655 -
Flags: feedback?(mkmelin+mozilla)
Attachment #9030656 -
Flags: review?(philipp)
Attachment #9030656 -
Flags: feedback?(mkmelin+mozilla)
Comment 8•6 years ago
|
||
Comment on attachment 9030656 [details] [diff] [review] calendar-day-label.patch Review of attachment 9030656 [details] [diff] [review]: ----------------------------------------------------------------- You didn't address the previous comments
Attachment #9030656 -
Flags: review?(philipp)
Attachment #9030656 -
Flags: feedback?(mkmelin+mozilla)
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #6) > Comment on attachment 9030655 [details] [diff] [review] > calendar-day-label.patch > > Review of attachment 9030655 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: calendar/base/content/calendar-base-view.js > @@ +24,5 @@ > > + > > + this.appendChild(this.longWeekdayName); > > + this.appendChild(this.shortWeekdayName); > > + > > + this.setAttribute("flex", "1"); > > would move this up, perhaps before this.textContent Ok > @@ +108,5 @@ > > + return 0; > > + } > > + > > + this.longWeekdayPixels = longNameWidth + > > + getSummarizedStyleValues(this.longName, ["margin-left", "margin-right"]); > > did you check eslint? getSummarizedStyleValues also appeears to be a global I donno why my eslin didn't complain
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #6) > Comment on attachment 9030655 [details] [diff] [review] > calendar-day-label.patch > > @@ +108,5 @@ > > + return 0; > > + } > > + > > + this.longWeekdayPixels = longNameWidth + > > + getSummarizedStyleValues(this.longName, ["margin-left", "margin-right"]); > > did you check eslint? getSummarizedStyleValues also appeears to be a global > This is because of https://searchfox.org/comm-central/source/calendar/.eslintrc.js#302..
Assignee | ||
Updated•6 years ago
|
Attachment #9030667 -
Flags: feedback?(mkmelin+mozilla)
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #9030667 -
Attachment is obsolete: true
Attachment #9030667 -
Flags: feedback?(mkmelin+mozilla)
Assignee | ||
Updated•6 years ago
|
Attachment #9031086 -
Flags: review?(philipp)
Attachment #9031086 -
Flags: feedback?(mkmelin+mozilla)
Comment 13•6 years ago
|
||
Comment on attachment 9031086 [details] [diff] [review] calendar-day-label.patch Review of attachment 9031086 [details] [diff] [review]: ----------------------------------------------------------------- Seems to work.
Attachment #9031086 -
Flags: feedback?(mkmelin+mozilla) → feedback+
Comment 14•6 years ago
|
||
Comment on attachment 9031086 [details] [diff] [review] calendar-day-label.patch Review of attachment 9031086 [details] [diff] [review]: ----------------------------------------------------------------- r+ with comments: ::: calendar/base/content/calendar-base-view.js @@ +67,5 @@ > + } > + > + get shortName() { > + return this.shortWeekdayName; > + } Do we need these extra getters, or can we just use longWeekdayName/shortWeekdayName? @@ +78,5 @@ > + } > + > + get weekDay() { > + return this.mWeekday; > + } Same here, can we just rename this.mWeekday to this.weekDay? ::: calendar/base/content/calendar-view-bindings.css @@ +3,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); > > +calendar-event-column { This file contains a bunch of unrelated whitespace changes. I don't think we should mix them in here.
Attachment #9031086 -
Flags: review?(philipp) → review+
Assignee | ||
Comment 15•6 years ago
|
||
Attachment #9031086 -
Attachment is obsolete: true
Attachment #9036531 -
Flags: review+
Attachment #9036531 -
Flags: feedback+
Assignee | ||
Comment 16•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 17•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8f9029b8f6f3
Migrate calendar-day-label binding to custom element. r=philipp
Updated•6 years ago
|
Target Milestone: --- → 6.8
You need to log in
before you can comment on or make changes to this bug.
Description
•