Closed Bug 988971 Opened 6 years ago Closed 4 years ago

Replace Classic with "Sandstone" skin as Standard skin

Categories

(Bugzilla :: User Interface, enhancement, P1)

4.5.2
enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 6.0

People

(Reporter: dkl, Assigned: u430950)

References

Details

(Keywords: relnote)

Attachments

(1 file, 9 obsolete files)

The default skin on bugzilla.mozilla.org is called Sandstone due to the similarities to the Sandstone Mozilla.org theme in use at the time. Several people have requested that the skin be made available in the upstream Bugzilla code so this bug is about tracking that.

Currently the following things will need to be taken into account when porting.

1. All of the CSS is located in skins/contrib/Mozilla/global.css and will need to be broken out into individual files such as global.css, enter_bug.css, show_bug.css, buglist.css, etc.
2. Changes are needed in to the global/header.html.tmpl specifically for the Sandstone skin currently. We either need to port those changes as-is or make changes to the standard header template that allows for all skins to share the same HTML and have the Sandstone skin achieve the same look using pure CSS and/or jscript.
3. The footer needs much improvement. One of the eventual goals of the Mozilla skin is to convert the save queries and other actions into vertical lists or drop down menus to make the footer cleaner.

If someone wants to take this one that would be great, otherwise BMO devels will be able to work on this after our other goals are completed.

dkl
We'd like to see this in Bugzilla 5.0.
Target Milestone: --- → Bugzilla 5.0
(In reply to David Lawrence [:dkl] from comment #0)
> 1. All of the CSS is located in skins/contrib/Mozilla/global.css and will
> need to be broken out into individual files such as global.css,
> enter_bug.css, show_bug.css, buglist.css, etc.

enter_bug.css and show_bug.css no longer exist in 5.0. This should make things easier. :)


> 3. The footer needs much improvement. One of the eventual goals of the
> Mozilla skin is to convert the save queries and other actions into vertical
> lists or drop down menus to make the footer cleaner.

Using a drop down menu is bug 324402, which I filed 8 years ago (wow!).
(In reply to Frédéric Buclin from comment #3)
> (In reply to David Lawrence [:dkl] from comment #0)
> > 1. All of the CSS is located in skins/contrib/Mozilla/global.css and will
> > need to be broken out into individual files such as global.css,
> > enter_bug.css, show_bug.css, buglist.css, etc.
> 
> enter_bug.css and show_bug.css no longer exist in 5.0. This should make
> things easier. :)

Ah good to know.
 
> > 3. The footer needs much improvement. One of the eventual goals of the
> > Mozilla skin is to convert the save queries and other actions into vertical
> > lists or drop down menus to make the footer cleaner.
> 
> Using a drop down menu is bug 324402, which I filed 8 years ago (wow!).

Yeah after writing this I thought that 3. may be best as a separate bug and can be used by any skin. So lets not do that part as part of this bug and get bug 324402 rebooted and work on that separately.

dkl
Priority: -- → P1
Whiteboard: [roadmap: 5.0]
Whiteboard: [roadmap: 5.0]
Attached patch patch.diff (obsolete) — Splinter Review
Patch consists of changes necessary to bring Sandstone functionality to all skins.
Attached file Sandstone.tar.gz (obsolete) —
Sandstone skin
Assignee: ui → theycallmefish
Status: NEW → ASSIGNED
Attached patch sandstonev2.diff (obsolete) — Splinter Review
Cleaned up files and added the Sandstone contrib folder to the patch.
Attachment #8433435 - Attachment is obsolete: true
Attachment #8433436 - Attachment is obsolete: true
Attachment #8478432 - Flags: review?(glob)
All CSS files must have a MPL 2.0 license.
Keywords: relnote
Attached patch sandstonev3.diff (obsolete) — Splinter Review
Added MPL 2.0 license
Attachment #8478432 - Attachment is obsolete: true
Attachment #8478432 - Flags: review?(glob)
Attachment #8479134 - Flags: review?(glob)
Summary: Port the BMO's "Sandstone" skin to upstream Bugzilla adding to the choices of themes available → Port the BMO's "Sandstone" skin to upstream Bugzilla, and make it the new default skin
Comment on attachment 8479134 [details] [diff] [review]
sandstonev3.diff

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

this looks fantastic :)  nice work ryan.
i've tested it in IE 11.

- there's some commented out code which should be removed
- trailing whitespace also needs to be removed

- the default skin should be changed from dusk to sandstone
  - when the preference is created: Bugzilla/Install.pm line 70
  - change any existing sites currently using dusk to sandstone (sites using skins other than dusk shouldn't be touched)

::: skins/contrib/Sandstone/bug.css
@@ +55,5 @@
> +body[class*=bz_group_] {
> +    background-color: inherit;
> +    background-image: url(index/noise.png), -moz-linear-gradient(#d7d7ff, #f0f0ff 400px);
> +    background-image: url(index/noise.png), -webkit-linear-gradient(#d7d7ff, #f0f0ff 400px);
> +    background-image: url(index/noise.png), linear-gradient(#d7d7ff, #f0f0ff 400px);

index --> global (as per other comment about renaming the index subdir)

::: skins/contrib/Sandstone/global.css
@@ +9,5 @@
> +@font-face {
> +  font-family: 'Open Sans';
> +  font-style: normal;
> +  font-weight: 400;
> +  src: local('Open Sans'), local('OpenSans'), url(index/opensans.woff) format('woff');

all files referenced in global.css belong in a "global" subdirectory.
eg. url(global/opensans.woff)

@@ +150,5 @@
> +
> +#title {
> +    width: 150px;
> +    font-size: 120%;
> +}

the #title block doesn't look correctly aligned with the "Home" link.

i suggest changing the padding in #titles to
  padding: 10px 10px 0 10px;
and adding padding to #title:
  padding: 0px 0px 0px 5px;

@@ +191,5 @@
> +
> +#enter_bug	{ background: url(index/bugzilla-papericon.png) no-repeat; }
> +#query		{ background: url(index/bugzilla-magnifier.png) no-repeat; }
> +#account	{ background: url(index/bugzilla-person-alternate.png) no-repeat; }
> +#help		{ background: url(index/bugzilla-questionmark2.png) no-repeat; }

..except for these files, which actually belong in the index subdir (to match the standard skin's layout)

::: template/en/default/global/common-links.html.tmpl
@@ +40,4 @@
>    [% Hook.process('action-links') %]
>  
>    [% IF user.login %]
> +    <li class="dropdown">

this should only be displayed in the header; there's no need to duplicate it in the footer, and it displays off the page.

ie. only display when qs_suffix is "_top"

@@ +44,5 @@
> +      <span class="anchor">[% user.login FILTER html %]</span>
> +      <ul>
> +        [% IF user.showmybugslink %]
> +          [% filtered_username = user.login FILTER uri %]
> +          <li><a href="[% Param('mybugstemplate').replace('%userid%', filtered_username) %]">My [% terms.Bugs %]</a></li>

these <li>'s can be a little bit hard to hit; i think a little extra padding here would help.
Attachment #8479134 - Flags: review?(glob) → review-
Target Milestone: Bugzilla 5.0 → ---
Attached patch sandstonev4.diff (obsolete) — Splinter Review
All the changes requested in comment 10 have been corrected in this patch.
Attachment #8479134 - Attachment is obsolete: true
Attachment #8504134 - Flags: review?(glob)
Comment on attachment 8504134 [details] [diff] [review]
sandstonev4.diff

>diff --git a/skins/contrib/Dusk/global.css b/skins/contrib/Dusk/global.css

There is no need to duplicate rules which are exactly the same in standard/*.css. Due to cascading, Dusk/*.css should only contain new or changed rules. This also makes maintaince much easier.

The same comment applies to Standstone/*.css.
Attachment #8504134 - Flags: review?(glob) → review-
Too late for 5.0. Targetting for 6.0.
Target Milestone: --- → Bugzilla 6.0
Attached patch sandstonev5.diff (obsolete) — Splinter Review
Updated with removal of redundant .css changes.
Attachment #8504134 - Attachment is obsolete: true
Attachment #8548314 - Flags: review?(glob)
Attached patch sandstonev6.diff (obsolete) — Splinter Review
Forgot the change to the Bugzilla/Install.pm file in previous patch
Attachment #8548314 - Attachment is obsolete: true
Attachment #8548314 - Flags: review?(glob)
Attachment #8548950 - Flags: review?(glob)
Could you attach a tar.gz archive with all binary files in it (PNG, WOFF)? I know they are in attachment 8433436 [details], but this archive also contains CSS files which are already part of the patch.
... and it seems this archive doesn't have all the PNG and WOFF files, at least not in the right place. In your patch, there are both index/noise.png and global/noise.png. Is that intentional?
Comment on attachment 8548950 [details] [diff] [review]
sandstonev6.diff

>+++ b/template/en/default/global/common-links.html.tmpl

>+    [% IF qs_suffix == "_top" %]
>+        <li class="dropdown">

I'm not a fan to see these links being removed from the page footer. It's painful to have to jump to the top of the page. I see no reason to remove them from there. And I don't see why some links would remain (New - Search - Browse) while some others wouldn't (Administration - Preferences - Requests). Because the latter ones are used less often?

Also, why is the search box in the page footer smaller than the one in the page header?
Attachment #8548950 - Flags: review?(glob) → review?(justdave)
It's sad that this has got stuck :-| f1sh: are you interested in finishing it up, if we can find a reviewer?

Gerv
I'm more than willing to finish this bug once we have a reviewer.
We discussed this in the last Bugzilla meeting; there is a support cost to carrying more skins in core, and we aren't sure that we want Sandstone itself, as opposed to pinching its best ideas and incorporating them into an existing skin.

Gerv
Summary: Port the BMO's "Sandstone" skin to upstream Bugzilla, and make it the new default skin → Replace Classic with "Sandstone" skin as Standard skin
(In reply to Gervase Markham [:gerv] from comment #22)
> we aren't sure that we want Sandstone itself

I'm not sure what Sandstone is these days, but I would suggest to implement the "experimental UI" from BMO. I turned it on a few minutes ago, and I already love it. :) (yes I know, I don't say such things very frequently)

And personally, I would consider it as the 6th (and final) goal for 6.0 (see bug 1234917 comment 0 for the other 5 goals).


(In reply to Ryan Wilson [:f1sh] from comment #21)
> I'm more than willing to finish this bug once we have a reviewer.

I can review it.
The current plan of record here is that f1sh will port Sandstone to core, overwriting Classic (although perhaps retaining the name) and rebase Dusk on top of it. So we will then have two skins, Sandstone and Dusk, and the "experimental UI" from BMO can be ported to trunk as it works on top of Sandstone.

f1sh: can you confirm that I have outlined this correctly?

Gerv
(In reply to Frédéric Buclin from comment #23)
> (In reply to Gervase Markham [:gerv] from comment #22)
> > we aren't sure that we want Sandstone itself
> 
> I'm not sure what Sandstone is these days, but I would suggest to implement
> the "experimental UI" from BMO. I turned it on a few minutes ago, and I
> already love it. :) (yes I know, I don't say such things very frequently)
> 
> And personally, I would consider it as the 6th (and final) goal for 6.0 (see
> bug 1234917 comment 0 for the other 5 goals).
 This is what I was working on in my modal port branch, BTW. https://github.com/dylanwh/bugzilla/tree/modal?files=1
(In reply to Gervase Markham [:gerv] from comment #24)
> The current plan of record here is that f1sh will port Sandstone to core,
> overwriting Classic (although perhaps retaining the name) and rebase Dusk on
> top of it. So we will then have two skins, Sandstone and Dusk

Yes, this is my plan. I'm currently writing as if it is Classic, but it should be as easy as renaming a couple of default fields if we want to be rid of the Classic name.
Attached patch standard-sandstone.diff (obsolete) — Splinter Review
Here is the first patch moving Sandstone into the standard skin.
Attachment #8548950 - Attachment is obsolete: true
Attachment #8548950 - Flags: review?(justdave)
Attachment #8741018 - Flags: review?(justdave)
Attached patch standard-sandstone-v2.diff (obsolete) — Splinter Review
Noticed some missing files and odd header behavior. Submitting a new patch.
Attachment #8741018 - Attachment is obsolete: true
Attachment #8741018 - Flags: review?(justdave)
Attachment #8742993 - Flags: review?(justdave)
Comment on attachment 8742993 [details] [diff] [review]
standard-sandstone-v2.diff

Removing the review request, as a new patch is forthcoming with changes introduced in bug 1167270 comment 5
Attachment #8742993 - Flags: review?(justdave)
Attachment #8742993 - Flags: review?(justdave)
Comment on attachment 8742993 [details] [diff] [review]
standard-sandstone-v2.diff

items with class=throw_error are a little hard to read. I haven't noticed any other issues. Can you fix the colors on those?
Attachment #8742993 - Flags: review?(justdave) → review-
I will have a new patch ready next week.
Attachment #8742993 - Attachment is obsolete: true
Comment on attachment 8784992 [details] [review]
[bugzilla] thef1sh:sandstone > bugzilla:master

r=dyan
Attachment #8784992 - Flags: review+
To github.com:bugzilla/bugzilla.git
   b9c4001..4da53d8  master -> master
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Blocks: 1306455
You need to log in before you can comment on or make changes to this bug.