864352 - Implement base Sandstone CSS

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P2)

Description

[Matjaz Horvat [:mathjazz]]

Change base elmo CSS to Sandstone and check for regressions.

It should not break any subpage, even the most hidden ones like:
https://l10n.mozilla.org/dashboard/tree-status/gaia-v1_0_1

Comment 1

[Matjaz Horvat [:mathjazz]]

Develop as a feature branch.

Comment 2

[Axel Hecht [:Pike]]

FYI, there's some starting point in doing an OK jquery.ui theme in bug 753163, that might be a helpful addition to this bug. Even though we may want to land that as a different changeset.

Comment 3

[Matjaz Horvat [:mathjazz]]

Attachment: Implements base Sandstone CSS and adapts Django templates accordingly

Comment 4

[Matjaz Horvat [:mathjazz]]

Attachment: Implements base Sandstone CSS and adapts Django templates accordingly

More obsolete static files removed.

Patch feature branch pushed to my fork:
https://github.com/mathjazz/elmo/commits/feature/864352-implement-base-sandstone-css

Comment 5

[Axel Hecht [:Pike]]

Comment on attachment 760313 [details] [diff] [review]
Implements base Sandstone CSS and adapts Django templates accordingly

Thanks for pushing that to a branch. This is a great step forward. Generally looks like what I am hoping for.

I'm currently running this locally, and I find a few things:

jquery isn't found, you removed the old copy, but didn't reference the copy we have from base.html. Should be 1.8.3.js (no min)

The l10n logo isn't in the repo, and the Bold and Italic versions of OpenSans. The homepage doesn't show that, but http://localhost:8000/source/pushes/ misses them for me.

There might be a host of content in static like download and aurora logos that can be cleaned up.

The login popup can't be closed if you fail to log in or decide not to.

The content of the footer should just be our old footer content, you got a copy-n-paste there from the designers that we didn't correct.

There's some table formatting that got lost on the team pages, http://localhost:8000/teams/de etc, notably, table.standard {width: 100%}.

Probably similar comments for http://localhost:8000/shipping/dashboard?locale=de, table formatting got lost. And the sign-off pages.

http://localhost:8000/dashboard/tree-status/fx_aurora has some padding coming back in the histogram.

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 760313 [details] [diff] [review]
Implements base Sandstone CSS and adapts Django templates accordingly

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

::: static/css/style.css
@@ -424,5 @@
> -/* instead of relying on order, make this selector more prevalent
> -   so it take precendence over `table.standard tr` */
> -#main-content table.standard-borderless tr {
> -    border-top: none;
> -}

Not sure where we need this rule, but the other three table rules below are probably the ones we're missing for the various table details.

Comment 7

[Matjaz Horvat [:mathjazz]]

Attachment: Implements base Sandstone CSS and fixes issues from #c5

Comment 8

[Matjaz Horvat [:mathjazz]]

(In reply to Axel Hecht [:Pike] from comment #5)
> Comment on attachment 760313 [details] [diff] [review]
> Implements base Sandstone CSS and adapts Django templates accordingly
> 
> Thanks for pushing that to a branch. This is a great step forward. Generally
> looks like what I am hoping for.

Thanks for the feedback! I'm attaching a new patch.

> I'm currently running this locally, and I find a few things:
> 
> jquery isn't found, you removed the old copy, but didn't reference the copy
> we have from base.html. Should be 1.8.3.js (no min)
> 
> The l10n logo isn't in the repo, and the Bold and Italic versions of
> OpenSans. The homepage doesn't show that, but
> http://localhost:8000/source/pushes/ misses them for me.

Issues with jQuery, fonts and logo are fixed in the new patch.

> There might be a host of content in static like download and aurora logos
> that can be cleaned up.

True, but I didn't touch it for now.

> The login popup can't be closed if you fail to log in or decide not to.

It can be closed by clicking on Login again, but I also enabled closing the popup with Esc and on click outside the popup.

> The content of the footer should just be our old footer content, you got a
> copy-n-paste there from the designers that we didn't correct.

Fixed.

> There's some table formatting that got lost on the team pages,
> http://localhost:8000/teams/de etc, notably, table.standard {width: 100%}.
> 
> Probably similar comments for
> http://localhost:8000/shipping/dashboard?locale=de, table formatting got
> lost. And the sign-off pages.
> 
> http://localhost:8000/dashboard/tree-status/fx_aurora has some padding
> coming back in the histogram.

Table formatting should now look the same as before Sandstone was applied.

Comment 9

[Axel Hecht [:Pike]]

More comments:

It helps to add

table {
    border-collapse: collapse;
    border-spacing: 0;
}

to reset.less. That fixes a few annoying border bugs, like in the histogram.

The one thing that puzzles me is the lack of min-width in the first two columns in the sign-off view. I wasn't able to track that down to a cause.

It'd be nice if we could make 

#colophon .footer-license

take the space that we freed up by removing the other footer content.

Comment 10

[Axel Hecht [:Pike]]

Attachment: log-in popup behind header

There seems to be something wrong with the z-order when loggin in.

Also, if there's a server error on log-in, you can't try to log in again, unless you reload the page (not sure if that's a regression, though)

Comment 11

[Axel Hecht [:Pike]]

The jquery.ui dialogs on both the sign-offs page like http://localhost:8000/shipping/signoffs/de/fx22 and on the push query options on http://localhost:8000/source/pushes/ aren't centered anymore, but come up crippled and in the top-left. On the search one, the button 'search' label disappears.

Comment 12

[Axel Hecht [:Pike]]

code-formatting nit: please no tabs for indention, but 4 spaces by default, unless there's 2 spaces commonly used in the file right now. I've found this at least in style.css.

Comment 13

[Matjaz Horvat [:mathjazz]]

Attachment: Patch, addressing comments 9-12

Comment 14

[Axel Hecht [:Pike]]

Attachment: twisties not aligned no more

The twisties in teh diff view don't align at this point on branch.

Also, the privacy link should link to https://l10n.mozilla.org/privacy/

Comment 15

[Matjaz Horvat [:mathjazz]]

Attachment: Patch 4

Addressing comment 14 and correcting table cell widths in the sign-offs overview.

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 761143 [details] [diff] [review]
Patch 4

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

I don't think I get the changes to login_form, user_forms, and base template. Is there  reason why you shuffled those around?

Some files changed permission to 755, we should undo that. Notably assets in jquery.ui.

I'm torn on adding socialshare.less, as I don't think we use that. Also review which binary assets of sandstone we need? Like the aurora and beta icons, for sure.

::: apps/pushes/static/pushes/css/diff.css
@@ +51,5 @@
> +  margin-top: -8px;
> +}
> +.ui-accordion-icons .ui-accordion-header a {
> +    padding-left: 1.2em;
> +}
\ No newline at end of file

I'm wondering, did you try to change diff.html to work with the standard jquery.ui styles?

I probably mocked the html output of an accordion back in the days, and it might be better to port that forward then to port the css rules backward.

Nit, missing newline in this file.

::: apps/shipping/static/shipping/css/snippet.css
@@ +122,2 @@
>      visibility: hidden;
>  }

Curious, why did this fix the table column width on the team page?

::: static/css/style.css
@@ +86,4 @@
>      border-top: 1px solid #ccc;
>  }
>  /* instead of relying on order, make this selector more prevalent
> +so it take precendence over `table.standard tr` */

Please keep the indention here, so that the text is aligned.

Comment 17

[Matjaz Horvat [:mathjazz]]

(In reply to Axel Hecht [:Pike] from comment #16)

> ::: apps/shipping/static/shipping/css/snippet.css
> @@ +122,2 @@
> >      visibility: hidden;
> >  }
> 
> Curious, why did this fix the table column width on the team page?

In Sandstone, .hidden is defined as:

.hidden {
  position: absolute;
  left: -999em;
}

Which makes the element hidden by moving it far left.

But what we want in the case of sign off buttons is elements still being there, occupying the space, but not visible. It's like "display: none" vs. "visibility: hidden".

Comment 18

[Matjaz Horvat [:mathjazz]]

(In reply to Axel Hecht [:Pike] from comment #16)

> I don't think I get the changes to login_form, user_forms, and base
> template. Is there  reason why you shuffled those around?

1. user_forms.html is where the menu content is defined (Log in / Hello, Axel).

2. login_form.html is where the popup content is defined. It's split in 2 sections, one for the logged out and the other for logged in state.

As per bug 791685, log out button was moved from 1 to 2. Same for the server error, due to space constraints.

Comment 19

[Axel Hecht [:Pike]]

... also, we'll want a preprocessed CSS so that we don't need lessc on the webheads.

Comment 20

[Matjaz Horvat [:mathjazz]]

Attachment: Patch 5

Addresses comments 16 & 19:

* Restores file permissions.

* Removes unused Sandstone icons. I'm keeping alternative backgrounds and common icons for now. We might need them later and don't want to look for 3rd party ones instead.

* Removes all Sandstone .less files and uses one preprocessed sandstone-resp.css file instead.

* Adds new line at end of diff.css. Using the default jQuery UI code also changes design dramatically. We might want to drop jQuery UI for this simple use case, but that's another bug.

* Adds indentation in style.css.

Comment 21

[Axel Hecht [:Pike]]

Attachment: screenshot with horizontal overflow

Oh, now I'm getting into the nitty-gritty.

On pages that have horizontal overflow, the background doesn't cover the whole page.

Also, at the point where the navigation bar collapses (home, teams, etc), there's the bar image on the top left, which doesn't seem to be doing anything constructive.

Comment 22

[Axel Hecht [:Pike]]

I've seen that you removed the less files in favor of a processed .css file. I wonder if we should store the copy we use in lib/sandstone, or vendor-local/sandstone, with a readme on how to process them, and where to get the original files from. I suspect that would make updating them easier?

Comment 23

[Axel Hecht [:Pike]]

On the teams page, search is broken. It tries to insertRule into the document.styleSheets[0], which is tabzilla.css, and thus bails. Works fine if you use any other stylesheet, though.

Comment 24

[Axel Hecht [:Pike]]

... we have a similar pattern in apps/pushes/static/pushes/js/pushlog.js, but that iterates to find a dedicated stylesheet with the right title, and is thus OK.

Comment 25

[Matjaz Horvat [:mathjazz]]

Attachment: Patch 6

Addresses comments 21 - 24 (and IRC conversation):

* Disables responsive design, which will be addressed as a follow-up. This also gets rid of the mobile menu and horizontal overflow issues with the background.

* Adds a copy of vanilla Sandstone with instructions on how we use it to make our lives easier when updating.

* Makes team search work again.

* Changes file permissions for jquery-ui-1.10.3.custom.css.

Comment 26

[Matjaz Horvat [:mathjazz]]

Attachment: Patch 7: Implements base Sandstone

* Fixes comment issue in sandstone.css.

* Uses CSS instead of inline style for team search.

Comment 27

[Matjaz Horvat [:mathjazz]]

Attachment: Patch 8: Includes redesigned homepage

Comment 28

[Axel Hecht [:Pike]]

Comment on attachment 762128 [details] [diff] [review]
Patch 7: Implements base Sandstone

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

nice. 'nuff said.

r=me, we should get this onto develop as soon as we have the homepage redo.

Comment 29

[Matjaz Horvat [:mathjazz]]

Attachment: Patch 9: Adds redesigned homepage to Patch 7

Comment 30

[Axel Hecht [:Pike]]

Comment on attachment 762152 [details] [diff] [review]
Patch 9: Adds redesigned homepage to Patch 7

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

I've found a few items in the copy, and a nit on the trailing newline, but otherwise, this looks awesome.

::: apps/homepage/static/homepage/css/index.css
@@ +61,5 @@
> +}
> +
> +.content .blog-links li a:after {
> +    content:" »";
> +}
\ No newline at end of file

Trialing newline, please ;-)

Comment 31

[Axel Hecht [:Pike]]

Comment on attachment 762152 [details] [diff] [review]
Patch 9: Adds redesigned homepage to Patch 7

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

These are copy changes Jeff put into an etherpad.

::: apps/homepage/templates/homepage/index.html
@@ +40,5 @@
> +        <source src="//videos-cdn.mozilla.net/serv/drafts/FFxGlobal.mp4" type="video/mp4" />
> +    </video>
> +    <article>
> +        <h2>Users first, no matter where they are.</h2>
> +        <p>Mozilla's translation and localization (l10n) efforts spread Firefox and web throughout the world.</p>

<p>Mozilla's translation and localization (l10n) efforts spread Firefox and the web throughout the world.</p>

@@ +41,5 @@
> +    </video>
> +    <article>
> +        <h2>Users first, no matter where they are.</h2>
> +        <p>Mozilla's translation and localization (l10n) efforts spread Firefox and web throughout the world.</p>
> +        <p>No matter your language, technical skill, or education, there are many ways you can help us put users first, no matter what language they speak.</p>

<p>No matter your language, technical skill, or education, there are many ways you can help us put users first in your corner of the world.</p>

Comment 32

[Matjaz Horvat [:mathjazz]]

Attachment: Patch 10: Adds redesigned homepage to Patch 7

Addresses comments #c30 and #c31.

Comment 33

[Axel Hecht [:Pike]]

Comment on attachment 762205 [details] [diff] [review]
Patch 10: Adds redesigned homepage to Patch 7

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

One nit, and then we're good to go to develop.

I'd like the two patch queues to be squashed into one commit each, with a commit message referencing this bug for the first, and bugs 864357 and 864360 on the second.

::: apps/homepage/views.py
@@ +62,5 @@
>        'feed_items': feed_items,
>        'locales_first_half': locales_first_half,
>        'locales_second_half': locales_second_half,
>        'locales_rest_count': locales_rest_count,
> +

remove this extra empty line?

Comment 34

[Matjaz Horvat [:mathjazz]]

Attachment: Patch 11: Adds redesigned homepage to Patch 7

Removes that line.

Comment 35

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/0ce91c61577fab0c1909304118db0ef579a3663f
Bug 864352 - Implement base Sandstone CSS, r=pike

Comment 36

[Axel Hecht [:Pike]]

Deployed on develop, marking FIXED.

Comment 37

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/36f8f7ede53c443585ebb0e0c8fe3319a80e7534
bug 864352, follow-up: filenames in diff shouldn't be styled as links, rs=me

Comment 38

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/5afe8dab8f9a7a24334c3b5f3225feba8c32ae34
bug 864352, follow-up: filenames in diff shouldn't be styled as links, take two. rs=me
Now with better support for removed files.