Magnus Melin [:mkmelin] Reporter
	Comment 4 • 5 years ago

Comment on attachment 9069915 [details] [diff] [review]
Bug-1554648_de-xbl_calendar-subscriptions-richlistitem.patch

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

Looks good.

::: calendar/base/content/widgets/calendar-subscriptions-list.js
@@ +70,4 @@
>                  return true;
>              } else {
>                  return false;
>              }

can we just do 

  return checkbox.getAttribute("checked") == "true";

@@ +88,5 @@
> +            if (checkbox.getAttribute("disabled") == "true") {
> +                return true;
> +            } else {
> +                return false;
> +            }

just
  return checkbox.getAttribute("disabled") == "true";

@@ +99,5 @@
> +        }
> +    }
> +
> +    customElements.define("calendar-subscriptions-richlistitem", MozCalendarSubscriptionsRichlistitem,
> +        { "extends": "richlistitem" });

please no quotes around extends

	Philipp Kewisch [:Fallen] [:📆][:🧩]
	Comment 5 • 5 years ago

Comment on attachment 9069915 [details] [diff] [review]
Bug-1554648_de-xbl_calendar-subscriptions-richlistitem.patch

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

r=me with Magnus' comments considered.

::: calendar/base/content/widgets/calendar-subscriptions-list.js
@@ +24,2 @@
>  
> +            this.setAttribute("is", "calendar-subscriptions-richlistitem");

If the connectedCallback of this class is called, isn't it already a `calendar-subscriptions-richlistitem`? Do we need this setAttribute call?