Closed Bug 1512945 Opened Last year Closed 11 months ago

[de-xbl] Migrate calendar-day-label to custom element.

Categories

(Calendar :: Lightning Only, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: arshad, Assigned: arshad)

References

Details

Attachments

(1 file, 5 obsolete files)

No description provided.
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED
Attached patch calendar-day-label.patch (obsolete) — Splinter Review
Attachment #9030205 - Flags: review?(makemyday)
Attachment #9030205 - Flags: feedback?(mkmelin+mozilla)
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-
> 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)
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)
Attached patch calendar-day-label.patch (obsolete) — Splinter Review
Attachment #9030205 - Attachment is obsolete: true
Attachment #9030655 - Flags: review?(philipp)
Attachment #9030655 - Flags: feedback?(mkmelin+mozilla)
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
Attached patch calendar-day-label.patch (obsolete) — Splinter Review
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 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)
(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
(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..
Attached patch calendar-day-label.patch (obsolete) — Splinter Review
Attachment #9030656 - Attachment is obsolete: true
Attachment #9030667 - Flags: feedback?(mkmelin+mozilla)
Attached patch calendar-day-label.patch (obsolete) — Splinter Review
Attachment #9030667 - Attachment is obsolete: true
Attachment #9030667 - Flags: feedback?(mkmelin+mozilla)
Attachment #9031086 - Flags: review?(philipp)
Attachment #9031086 - Flags: feedback?(mkmelin+mozilla)
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 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+
Attachment #9031086 - Attachment is obsolete: true
Attachment #9036531 - Flags: review+
Attachment #9036531 - Flags: feedback+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8f9029b8f6f3
Migrate calendar-day-label binding to custom element. r=philipp

Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.