1538485 - Remove the <grid> from the calendar's preferences

Status: RESOLVED FIXED

(Calendar :: Preferences, task)

Description

[Richard Marti (:Paenglab)]

See bug 1521483 why this needs to be removed.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1538485-no-grid-calendar-prefs.patch

Whoever is first. One review is enough.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 9053056 [details] [diff] [review]
1538485-no-grid-calendar-prefs.patch

Yep, whoever is first :-)

Comment 3

[Geoff Lankow (:darktrojan)]

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.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: 1538485-no-grid-calendar-prefs.patch

(In reply to Geoff Lankow (:darktrojan) from comment #3)

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.

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

[Geoff Lankow (:darktrojan)]

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.

Comment 6

[Richard Marti (:Paenglab)]

Attachment: 1538485-no-grid-calendar-prefs-variant.patch

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

[Geoff Lankow (:darktrojan)]

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.

Comment 8

[Richard Marti (:Paenglab)]

Attachment: 1538485-no-grid-calendar-prefs-variant.patch

(In reply to Geoff Lankow (:darktrojan) from comment #7)

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.

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.

Comment 9

[Richard Marti (:Paenglab)]

(In reply to Geoff Lankow (:darktrojan) from comment #7)

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.

Comment 10

[Geoff Lankow (:darktrojan)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/187544e30b40
Remove grid usage from calendar's preferences. r=darktrojan