Remove the <grid> from the calendar's preferences
Categories
(Calendar :: Preferences, task)
Tracking
(Not tracked)
People
(Reporter: Paenglab, Assigned: Paenglab)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Paenglab
:
review+
|
Details | Diff | Splinter Review |
See bug 1521483 why this needs to be removed.
Assignee | ||
Comment 1•6 years ago
|
||
Whoever is first. One review is enough.
Comment 2•6 years ago
|
||
Comment on attachment 9053056 [details] [diff] [review] 1538485-no-grid-calendar-prefs.patch Yep, whoever is first :-)
Updated•6 years ago
|
Comment 3•6 years ago
|
||
Comment on attachment 9053056 [details] [diff] [review] 1538485-no-grid-calendar-prefs.patch Review of attachment 9053056 [details] [diff] [review]: ----------------------------------------------------------------- Mixing XUL and HTML like this is something I am really not a fan of but I guess we do not really have a choice. ::: calendar/base/content/preferences/alarms.xul @@ +27,5 @@ > > <groupbox> > <label><html:h2>&pref.alarmgoesoff.label;</html:h2></label> > + <hbox id="alarm-sound-box"> > + <vbox align="top"> "top" isn't a valid value for align. I think what you're trying to do is pack="start", which is unnecessary anyway. @@ +33,5 @@ > + preference="calendar.alarms.playsound" > + label="&pref.playasound;" > + accesskey="&pref.calendar.alarms.playsound.accessKey;"/> > + </vbox> > + <vbox id="alarm-sound-table" align="top"> Same here. ::: calendar/base/content/preferences/general.xul @@ +66,5 @@ > </groupbox> > > <groupbox id="defaults-itemtype-groupbox"> > <label><html:h2 id="defaults-itemtype-caption">&pref.defaults.label;</html:h2></label> > + <vbox id="defaults-itemtype-box" align="top"> Why are we switching to vbox? In fact, it's not your fault, but I dislike this whole section. Can we not have UI that depends on the state of a menulist? Maybe a job for another bug. (Also you want align="start", the existing code is also wrong.) @@ +80,5 @@ > </menupopup> > </menulist> > <spacer id="defaults-itemtype-spacer" flex="1"/> > + <deck id="defaults-itemtype-deck" class="indent"> > + <hbox id="defaults-event-grid" align="top"> align="start" ::: calendar/base/themes/common/calendar-preferences.css @@ +32,5 @@ > + > +#eventdefalarm, > +#tododefalarm, > +#alarmSoundFileField { > + width: calc(100% - 8px); /* 8px is the sum of the element margins. */ The devtools say the margin is 12px for the menulists, but this does work so let's just ignore it.
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Geoff Lankow (:darktrojan) from comment #3)
Comment on attachment 9053056 [details] [diff] [review]
1538485-no-grid-calendar-prefs.patchReview of attachment 9053056 [details] [diff] [review]:
Mixing XUL and HTML like this is something I am really not a fan of but I
guess we do not really have a choice.
Me too. It makes a lot of flexing issues.
::: calendar/base/content/preferences/alarms.xul
@@ +27,5 @@<groupbox> <label><html:h2>&pref.alarmgoesoff.label;</html:h2></label>
<hbox id="alarm-sound-box">
<vbox align="top">
"top" isn't a valid value for align. I think what you're trying to do is
pack="start", which is unnecessary anyway.
Wasn't needed here, removed.
@@ +33,5 @@
preference="calendar.alarms.playsound"
label="&pref.playasound;"
accesskey="&pref.calendar.alarms.playsound.accessKey;"/>
</vbox>
<vbox id="alarm-sound-table" align="top">
Same here.
Same here, removed.
::: calendar/base/content/preferences/general.xul
@@ +66,5 @@</groupbox> <groupbox id="defaults-itemtype-groupbox"> <label><html:h2 id="defaults-itemtype-caption">&pref.defaults.label;</html:h2></label>
<vbox id="defaults-itemtype-box" align="top">
Why are we switching to vbox? In fact, it's not your fault, but I dislike
this whole section. Can we not have UI that depends on the state of a
menulist? Maybe a job for another bug.
I had flexing and aligning issues when it was a hbox.
And yes, for another bug.
(Also you want align="start", the existing code is also wrong.)
Fixed.
@@ +80,5 @@
</menupopup> </menulist> <spacer id="defaults-itemtype-spacer" flex="1"/>
<deck id="defaults-itemtype-deck" class="indent">
<hbox id="defaults-event-grid" align="top">
align="start"
Fixed.
Comment 5•6 years ago
|
||
Comment on attachment 9053508 [details] [diff] [review] 1538485-no-grid-calendar-prefs.patch Nice. Please file the follow-up so that we don't forget to do it.
Assignee | ||
Comment 6•6 years ago
|
||
During writing the new bug I had an idea. What do you think about showing the options always? With the in-content prefs we have enough space.
Comment 7•6 years ago
|
||
Comment on attachment 9053513 [details] [diff] [review] 1538485-no-grid-calendar-prefs-variant.patch That's pretty much what I had in mind. I think it looks better without the indent. While we're here, the `<table>`s have a default cell spacing of 2, which I think is unwanted. Let's get rid of that, in aboutPreferences.css, I guess.
Assignee | ||
Comment 8•6 years ago
|
||
(In reply to Geoff Lankow (:darktrojan) from comment #7)
Comment on attachment 9053513 [details] [diff] [review]
1538485-no-grid-calendar-prefs-variant.patchThat's pretty much what I had in mind. I think it looks better without the
indent.
Removed the indent.
While we're here, the
<table>
s have a default cell spacing of 2, which I
think is unwanted. Let's get rid of that, in aboutPreferences.css, I guess.
I had it already with the border-spacing: 0; in calendar-preferences.css.
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Geoff Lankow (:darktrojan) from comment #7)
Comment on attachment 9053513 [details] [diff] [review]
1538485-no-grid-calendar-prefs-variant.patchThat's pretty much what I had in mind. I think it looks better without the
indent.While we're here, the
<table>
s have a default cell spacing of 2, which I
think is unwanted. Let's get rid of that, in aboutPreferences.css, I guess.
Comment 10•6 years ago
|
||
I had it already with the border-spacing: 0; in calendar-preferences.css.
My build was missing the new file. I'd already fallen for that once today.
Comment 11•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/187544e30b40
Remove grid usage from calendar's preferences. r=darktrojan
Updated•6 years ago
|
Updated•5 years ago
|
Description
•