Paul Morris [:pmorris]
	Comment 2 • 5 years ago

Comment on attachment 9099570 [details] [diff] [review]
Bug-1582717_remove-frid-calendar-month-base-view.patch

Review of attachment 9099570 [details] [diff] [review]:
-----------------------------------------------------------------

Code changes look good and the grid views look like they are working fine.  Just a few comments on the code to address.

::: calendar/base/content/calendar-month-base-view.js
@@ +433,4 @@
>        // week labels, taking into account whether days-off are displayed or not.
>        let weekLabelColumnPos = -1;
>  
> +      const rows = this.monthgrid.childNodes;

Better to go ahead and use `.children` instead of `.childNodes` here and elsewhere in this patch.  (It's friendlier for use with HTML, see bug 1570954).

@@ +451,1 @@
>          }

Lets simplify this while we're here:

row.toggleAttribute("hidden", finished);
if (finished) {
  continue;
}

@@ +600,5 @@
> +        for (let j = 0; j < rows[0].childNodes.length; j++) {
> +          if (rows[j]) {
> +            rows[j].childNodes[i].toggleAttribute("hidden", dayOff && !this.mDisplayDaysOff);
> +          }
> +        }

Here's a way to simplify and make this easier to follow:

const lastColNum = rows[0].children.length - 1;
for (let colNum = 0; colNum <= lastColNum; colNum++) {
  ...
  for (let row of rows) {
    row.children[colNum].toggleAttribute(...);
  }
}

	Paul Morris [:pmorris]
	Comment 4 • 5 years ago

Comment on attachment 9099968 [details] [diff] [review]
Bug-1582717_remove-frid-calendar-month-base-view-1.patch

Review of attachment 9099968 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.  I checked again and the month view grid is still intact.