Last Comment Bug 1192854 - change tabs on user preferences from horizontal to vertical layout
: change tabs on user preferences from horizontal to vertical layout
Status: RESOLVED FIXED
:
Product: bugzilla.mozilla.org
Classification: Other
Component: User Interface (show other bugs)
: Production
: Unspecified Unspecified
-- normal (vote)
: ---
Assigned To: David Lawrence [:dkl]
:
:
Mentors:
Depends on:
Blocks: 1192687 1235657
  Show dependency treegraph
 
Reported: 2015-08-10 08:09 PDT by Byron Jones ‹:glob›
Modified: 2015-12-29 15:37 PST (History)
4 users (show)
See Also:
Due Date:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
1192854_1.patch (3.12 KB, patch)
2015-08-10 20:40 PDT, David Lawrence [:dkl]
glob: review-
Details | Diff | Splinter Review
1192854_2.patch (3.32 KB, patch)
2015-08-11 13:01 PDT, David Lawrence [:dkl]
glob: review+
Details | Diff | Splinter Review

Description User image Byron Jones ‹:glob› 2015-08-10 08:09:37 PDT
the current count of tabs on the user preferences page is large, and causes layout issues on smaller screens.

it would look much better if the page selectors were vertical on the left hand side of the page.
Comment 1 User image David Lawrence [:dkl] 2015-08-10 20:40:17 PDT
Created attachment 8646130 [details] [diff] [review]
1192854_1.patch
Comment 2 User image Byron Jones ‹:glob› 2015-08-10 23:52:28 PDT
Comment on attachment 8646130 [details] [diff] [review]
1192854_1.patch

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

this looks like a great start (i really like the 50% opaque menu background)
- the background of the main content should be white
- there's a unnecessary 40px left padding on the <ul> element - the menu should be flush with the left border of bugzilla-body
- the menu is too wide; 175px looks about right to me (with a 5px gap between it and the content)

::: skins/standard/admin.css
@@ +185,5 @@
> +    margin-top: -1em;
> +}
> +
> +#prefnav li {
> +    font-size: 12px;

i think it would be better to leave the font-size as the default, and tighten up the padding/margins to gain more content space.

::: template/en/default/account/prefs/tabs.html.tmpl
@@ +17,5 @@
> +  #
> +  # Contributor(s): Gervase Markham <gerv@gerv.net>
> +  #                 Myk Melez <myk@mozilla.org>
> +  #                 Marc Schumann <wurblzap@gmail.com>
> +  #%]

this is the wrong license

@@ +35,5 @@
> +        [% IF tab.name == current_tab_name %]
> +          <li class="selected">[% tab.label FILTER html %]</li>
> +        [% ELSE %]
> +          <li><a href="[% tab.link FILTER html %]">[% tab.label FILTER html %]</a></li>
> +        [% END %]

i've been wanting to see something changed here for ages .. since we're touching this code can you make the selected tab's name clickable please.  it should look the same as the current (eg. black text).
Comment 3 User image David Lawrence [:dkl] 2015-08-11 13:01:34 PDT
Created attachment 8646545 [details] [diff] [review]
1192854_2.patch
Comment 4 User image Byron Jones ‹:glob› 2015-08-12 00:02:48 PDT
Comment on attachment 8646545 [details] [diff] [review]
1192854_2.patch

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

this looks great, r=glob with comments addressed on commit.

::: skins/standard/admin.css
@@ +184,5 @@
> +}
> +
> +#prefnav {
> +    width: 14em;
> +    float:left;

you're missing a space after that :

@@ +196,5 @@
> +}
> +
> +#prefnav li {
> +    padding: .6em 1em;
> +    background: rgba(255, 255, 255, 0.5) none repeat scroll 0% 0%;

this should just be:
> background: rgba(255, 255, 255, 0.5)

@@ +209,5 @@
> +    text-decoration: underline;
> +}
> +
> +#prefnav li.selected {
> +    background: #FFF none repeat scroll 0% 0%;

that should be
> background: #fff

using the classic theme it's impossible to determine which <li> is selected, and it's barely possible with dusk.

adding "font-weight: bold" appears to be sufficient.

@@ +216,5 @@
> +#prefcontent {
> +    margin-left: 15em;
> +    padding: .2em 1.5em 1.5em 1.5em;
> +    box-shadow: 1px 1px 5px rgba(0, 0, 0, 0.1);
> +    background: #FFF none repeat scroll 0% 0%;

> background: #fff

::: template/en/default/filterexceptions.pl
@@ +202,5 @@
>  ],
>  
> +'account/prefs/tabs.html.tmpl' => [
> +  'content',
> +],

instead of a filter exception, please change tabs.html.tmpl to use "content FILTER none".
Comment 5 User image David Lawrence [:dkl] 2015-08-12 07:00:42 PDT
Thanks!

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   56c61ae..b109a7d  master -> master

Note You need to log in before you can comment on or make changes to this bug.