Closed Bug 1582717 Opened 5 months ago Closed 5 months ago

remove grid usage from comm/calendar/base/content/calendar-month-base-view.js

Categories

(Calendar :: Calendar Views, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: khushil324, Assigned: khushil324)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → khushil324
Depends on: tb-burn-xul-grids
No longer depends on: tb-burn-xul-grids
Status: NEW → ASSIGNED
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(...);
  }
}
Attachment #9099570 - Flags: review?(paul)
Attachment #9099570 - Attachment is obsolete: true
Attachment #9099968 - Flags: review?(paul)
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.
Attachment #9099968 - Flags: review?(paul) → review+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/311c42a94e23
remove grid usage from calendar-month-base-view.js. r=pmorris

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