Clean up style.css and undo generic rules such as table stuff

RESOLVED FIXED in 2.1

Status

P1
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: peterbe, Assigned: peterbe)

Tracking

Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
* Every table should define a class.

* Remove the the quick-search selectors

* Some "teams" specific selectors --> teams.css (audit for more like this)

* Review the use of the `*` selector
Marking this as P1, this should block merging dev to prod.
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
(Assignee)

Comment 2

7 years ago
Created attachment 607631 [details] [diff] [review]
style.css cleaned up

No default table styling. 
License header on style.css
Introduced dedicated index.css for the home page. 
Cell borders look great on the dashboard.
Attachment #607631 - Flags: review?(l10n)
Comment on attachment 607631 [details] [diff] [review]
style.css cleaned up

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

r-, I think the rules are still bleeding back and forth, and most notably, the display:block rule is still too generic and has hacks to revert it.

Also, there seem to be a lot of rules that need to be put in for the exhibit table views. I'd rather have the rules they need to overwrite to be more specific than having css going back and forth.

::: apps/shipping/static/shipping/css/dashboard.css
@@ +134,1 @@
>  }

Many of these seem to be hacks against too generic rules elsewhere.

I'm also concerned about the descendant combinators for tr, td, th, can we make those explicit child selectors?

::: static/css/style.css
@@ +411,5 @@
>  /* Footer */
>  .push {
>      clear: both;
>      height: 75px;
>  }

This rule doesn't seem to belong here, I think. The only class="push" thing I find ad-hoc is in the signoffs view, so probaby this should be in signoffs.css?

@@ +467,3 @@
>      padding: 10px 7px;
>      vertical-align: top;
>  }

This sounds like another rule that would benefit from not having a descendent combinator. Also, I'm not sure that td padding is generally wanted.

@@ +471,2 @@
>      display: block;
>  }

This rule should go, and only ever be used in dedicated td's, I guess. We hack around that being here all over the place.

Where does this rule actually do what it should? I'm really confused why we'd use a span if we want a block display.

@@ +475,3 @@
>      text-align: center;
>      vertical-align: middle;
>  }

Where do these take effect? My attempts to find them failed so far.
Attachment #607631 - Flags: review?(l10n) → review-
Comment on attachment 607631 [details] [diff] [review]
style.css cleaned up

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

::: apps/shipping/static/shipping/css/dashboard.css
@@ +128,5 @@
> +    background-image:     -ms-linear-gradient(top, white, #eee);
> +    background-image:      -o-linear-gradient(top, white, #eee);
> +    background-image:         linear-gradient(top, white, #eee);
> +}
> +table.exhibit-tabularView-body td.bugs_cell span {

Here we are coding around the td span { display: block; } rule. If we got rid of that one, we can get rid of this one.
Comment on attachment 607631 [details] [diff] [review]
style.css cleaned up

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

::: static/css/style.css
@@ +411,5 @@
>  /* Footer */
>  .push {
>      clear: both;
>      height: 75px;
>  }

This was related to the way footer stick used to work but is no longer required. i believe we can get rid of this.
Comment on attachment 607631 [details] [diff] [review]
style.css cleaned up

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

::: static/css/style.css
@@ +475,3 @@
>      text-align: center;
>      vertical-align: middle;
>  }

I can also not find any reference to this in CSS as well as JS files.
(Assignee)

Comment 7

7 years ago
Created attachment 608880 [details] [diff] [review]
stuff removed and stuff fixed

Thank you for your patience Axel!

It's getting a bit messy here in the bug but I hope I managed to address all issues. 

* I'm not going to touch on the preferred padding on `table.standard td`. It's not "wrong" and is part of the design rather. 

* With this patch, the sign-off view now looks great. The odd top-left & bottom-left rounded borders on the left-most column still doesn't work great. I chalk that up to being the wrong way entirely to do such a design. What we're getting now is at least really usable. http://cl.ly/0a0Y0f2s0U1l2y2S3Q1O
Attachment #607631 - Attachment is obsolete: true
Attachment #608880 - Flags: review?(l10n)
Comment on attachment 608880 [details] [diff] [review]
stuff removed and stuff fixed

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

r=me, thanks for hanging in on this.
Attachment #608880 - Flags: review?(l10n) → review+
(Assignee)

Comment 9

7 years ago
https://github.com/mozilla/elmo/commit/8fad6ef8af70eb4be94fa768da05a8fd55669e12
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Duplicate of this bug: 729612

Updated

7 years ago
Assignee: nobody → peterbe
Target Milestone: --- → 2.1
You need to log in before you can comment on or make changes to this bug.