Bugzilla

(In reply to Alessandro Castellani (:aleca) from comment #23)

> (In reply to Philipp Kewisch [:Fallen] [:📆] from comment #22)
> > I've reviewed the calendar files, assuming you have r+ from Magnus for mail.
> > I've noticed a few more places you changed indent or wrapping in the mail
> > code. For line wrapping. Are you aiming at 80 or 100? We've been aiming for
> > 100 in calendar.
> 
> Yes, the patch has been `r+` by Magnus.
> I'm aiming at 80 char limit as recommended by Magnus, should I stick to 100 for the calendar?
Magnus, is this something you want to hold on to for mail? I think 80 is pretty limiting, and the standard terminal is a bit wider nowadays. Let's aim for 100 in calendar though.


> 
> > I think you might be able to use XPCOMUtils.defineLazyGetter() here.
> 
> I've never used it, but I can read the docs and implement it if you think it's necessary.
It would probably simplify the getter here a bit. It basically does what you've done manually, and XPCOMUtils should be almost everywhere.

> 
> > Since this code is repeated pretty often, do you think it makes sense to add
> > a helper somewhere?
> 
> We could, but I'd suggest to do it in a dedicated bug. Right now, during creation of a new `MozElements.NotificationBox`, different attributes and selectors are used each time. A helper method would be useful indeed, but would be better to handle it on a dedicated patch since it needs to be modular and usable throughout the entire app.
> What do you think?
The way we've usually been doing this is creating the helper for the purpose it is used for, and if others need to use it then adapt. The uses in this patch are already enough that I'd consider a helper.

(In reply to Alessandro Castellani (:aleca) from comment #23)

> I'm aiming at 80 char limit as recommended by Magnus, should I stick to 100 for the calendar?

Magnus, is this something you want to hold on to for mail? I think 80 is pretty limiting, and the standard terminal is a bit wider nowadays. Let's aim for 100 in calendar though.


> > I think you might be able to use XPCOMUtils.defineLazyGetter() here.
> 
> I've never used it, but I can read the docs and implement it if you think it's necessary.

It would probably simplify the getter here a bit. It basically does what you've done manually, and XPCOMUtils should be almost everywhere.


> We could, but I'd suggest to do it in a dedicated bug. Right now, during creation of a new `MozElements.NotificationBox`, different attributes and selectors are used each time. A helper method would be useful indeed, but would be better to handle it on a dedicated patch since it needs to be modular and usable throughout the entire app.
> What do you think?

The way we've usually been doing this is creating the helper for the purpose it is used for, and if others need to use it then adapt. The uses in this patch are already enough that I'd consider a helper.

(In reply to Alessandro Castellani (:aleca) from comment #23)

> I'm aiming at 80 char limit as recommended by Magnus, should I stick to 100 for the calendar?

Magnus, is this something you want to hold on to for mail? I think 80 is pretty limiting, and the standard terminal is a bit wider nowadays. Let's aim for 100 in calendar though.


> We could, but I'd suggest to do it in a dedicated bug. Right now, during creation of a new `MozElements.NotificationBox`, different attributes and selectors are used each time. A helper method would be useful indeed, but would be better to handle it on a dedicated patch since it needs to be modular and usable throughout the entire app.
> What do you think?

The way we've usually been doing this is creating the helper for the purpose it is used for, and if others need to use it then adapt. The uses in this patch are already enough that I'd consider a helper.