Review of attachment 9080582 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure about this approach. What's the reason behind going with divs instead of hbox and vbox? I'm not against having a generic gridLayout file to use when necessary, but I think we should first decide which approach we want to follow between flexbox and CSS grid. ::: mail/themes/shared/mail/gridLayout.css @@ +1,4 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > +.threeColumnGrid { If we're going with this approach, I think we should rename the classes to list the main property first and then the modifier. eg. grid-three-column. @@ +10,5 @@ > + display:inline-grid; > + grid-template-columns: auto auto; > +} > + > +table.threeColumnGrid textbox, menulist { I don't seem to find the table element used here, why the class applied to it? Also, don't style a common element like menulist without a proper selector or parent-child relationship, otherwise all the menulist elements will be affected if we include this file. @@ +14,5 @@ > +table.threeColumnGrid textbox, menulist { > + width: 100%; > +} > + > +table.twoColumnGrid textbox, menulist { Why the repetition? Merge this declaration with the one above. @@ +18,5 @@ > +table.twoColumnGrid textbox, menulist { > + width: 100%; > +} > + > +.divAlignCenter { This should also be .flex-items-center ::: mailnews/addrbook/prefs/content/pref-directory-add.xul @@ +35,4 @@ > > <tabpanels id="directoryTabPanels" flex="1"> > <vbox> > + <div xmlns="http://www.w3.org/1999/xhtml" class="threeColumnGrid"> Any particular reason why we're implementing divs instead of hbox and vbox? @@ +144,5 @@ > + <xul:label value="&saslMechanism.label;" control="saslMechanism" > + accesskey="&saslMechanism.accesskey;"/> > + </div> > + <div> > + <xul:menulist id="saslMechanism" style="width:100%;"> Do you need the inline style if you're applying the width:100% attribute in the CSS file?
Bug 1557216 Comment 7 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
Review of attachment 9080582 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure about this approach. What's the reason behind going with divs instead of hbox and vbox? I'm not against having a generic gridLayout file to use when necessary, but I think we should first decide which approach we want to follow between flexbox and CSS grid. ::: mail/themes/shared/mail/gridLayout.css @@ +1,4 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > +.threeColumnGrid { If we're going with this approach, I think we should rename the classes to list the main property first and then the modifier. eg. grid-three-column. @@ +10,5 @@ > + display:inline-grid; > + grid-template-columns: auto auto; > +} > + > +table.threeColumnGrid textbox, menulist { I don't seem to be able to find the table element used here, why the class applied to it? Also, don't style a common element like menulist without a proper selector or parent-child relationship, otherwise all the menulist elements will be affected if we include this file. @@ +14,5 @@ > +table.threeColumnGrid textbox, menulist { > + width: 100%; > +} > + > +table.twoColumnGrid textbox, menulist { Why the repetition? Merge this declaration with the one above. @@ +18,5 @@ > +table.twoColumnGrid textbox, menulist { > + width: 100%; > +} > + > +.divAlignCenter { This should also be .flex-items-center ::: mailnews/addrbook/prefs/content/pref-directory-add.xul @@ +35,4 @@ > > <tabpanels id="directoryTabPanels" flex="1"> > <vbox> > + <div xmlns="http://www.w3.org/1999/xhtml" class="threeColumnGrid"> Any particular reason why we're implementing divs instead of hbox and vbox? @@ +144,5 @@ > + <xul:label value="&saslMechanism.label;" control="saslMechanism" > + accesskey="&saslMechanism.accesskey;"/> > + </div> > + <div> > + <xul:menulist id="saslMechanism" style="width:100%;"> Do you need the inline style if you're applying the width:100% attribute in the CSS file?