Arshad Khan [:arshad] Assignee
	Description • 6 years ago

      No description provided.

	Magnus Melin [:mkmelin]
	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)

	Arshad Khan [:arshad] 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?

	Magnus Melin [:mkmelin]
	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

	Magnus Melin [:mkmelin]
	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

	Magnus Melin [:mkmelin]
	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

	Arshad Khan [:arshad] 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

	Arshad Khan [:arshad] 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..

	Magnus Melin [:mkmelin]
	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.

	Philipp Kewisch [:Fallen] [:📆][:🧩]
	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.