See #1995272: [Meta] Refactor module CSS files inline with our CSS standards

Remaining tasks

Review the current CSS — What to look for when reviewing CSS

User interface changes

None

API changes

None

CommentFileSizeAuthor
#16 2408467-12-14.txt402 bytesDickJohnson
#16 2408467-14.patch1.77 KBDickJohnson
#13 2408467-12-bartik-rtl-after.png14.95 KBidebr
#13 2408467-12-bartik-after.png14.5 KBidebr
#13 2408467-12-seven-rtl-after.png2.02 KBidebr
#13 2408467-12-seven-after.png2.08 KBidebr
#13 2408467-12-stark-rtl-after.png3.57 KBidebr
#13 2408467-12-stark-after.png3.45 KBidebr
#12 2408467-12.patch1.8 KBidebr
#10 pager-1887862-10.patch1.74 KBDickJohnson
#10 1887862-7.10.txt783 bytesDickJohnson
#7 interdiff-1887662-5-7.txt1.23 KBDickJohnson
#7 pager-1887862-7.patch1.66 KBDickJohnson
#6 bartik.pager_.css_.txt448 byteslewisnyman
#6 system.pager_.css_.txt256 byteslewisnyman
#6 seven.pager_.css_.txt842 byteslewisnyman
#5 pager-1887862-5.patch822 bytesmonobasic

Comments

lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes
DickJohnson’s picture

DickJohnson commented
Assigned: Unassigned » DickJohnson

Starting to work on this at Brighton sprints.

DickJohnson’s picture

DickJohnson commented
Assigned: DickJohnson » Unassigned

After a small chat here, decided not to work on this after all.

monobasic’s picture

monobasic commented
Assigned: Unassigned » monobasic
monobasic’s picture

monobasic commented
Assigned: monobasic » Unassigned
Status: Active » Needs review
StatusFileSize
new pager-1887862-5.patch822 bytes
lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Needs work
StatusFileSize
new seven.pager_.css_.txt842 bytes
new system.pager_.css_.txt256 bytes
new bartik.pager_.css_.txt448 bytes 
  1. +++ b/core/themes/seven/css/components/pager.css
@@ -16,11 +17,12 @@
   -webkit-transition: border-bottom-color 0.2s;
+  -moz-transition: border-bottom-color 0.2s;
+  -o-transition: border-bottom-color 0.2s;

    Based on caniuse.com I don't think we need these prefixes, we should removed the -webkit-prefix as well 

  2. +++ b/core/themes/seven/css/components/pager.css
@@ -16,11 +17,12 @@
-  -webkit-font-smoothing: antialiased;

    It seems like we've removed this property, I think we need it?

Apart from that, the Seven CSS looks pretty solid. Let's not forget to look over the system CSS and Bartik CSS. I've uploaded the files so they can be reviewed with Dreditor

DickJohnson’s picture

DickJohnson commented
StatusFileSize
new pager-1887862-7.patch1.66 KB
new interdiff-1887662-5-7.txt1.23 KB

Looked a bit on this.

1. Removed list-style-type and image from system.theme.css as couln't find out where these would be affecting
2. Changed color hexas to small letters
3. Fixed #6.1

DickJohnson’s picture

DickJohnson commented
Status: Needs work » Needs review
idebr’s picture

idebr commented
Status: Needs review » Needs work

Thanks for working on this, DickJohnson. The pager css looks very clean, I found just a few minor issues:

  1. +++ b/core/themes/bartik/css/components/pager.css
@@ -1,4 +1,7 @@
 .pager .pager__items {

    This selector is overqualified to win specificity over .region-content ul. Let's add a comment to explain this, so it doesn't get removed without a proper review. 

  2. +++ b/core/themes/seven/css/components/pager.css
@@ -1,5 +1,6 @@
+ * Styles for Seven's Pagination

    The comments needs to end in a full stop.

DickJohnson’s picture

DickJohnson commented
Status: Needs work » Needs review
StatusFileSize
new 1887862-7.10.txt783 bytes
new pager-1887862-10.patch1.74 KB

Fixed #9.1 and #9.2. Also updated Bartik's comment a bit so that it's consistent with Seven's comment. We're talking about same thing anyways.

idebr’s picture

idebr commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Changes look good! Patch only needs a reroll now that #2398447: Remove the "typography" CSS file in Bartik has been committed.

idebr’s picture

idebr commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2408467-12.patch1.8 KB

Straight rerolled patch attached. I'll have a more in depth look to review its contents.

idebr’s picture

idebr commented
StatusFileSize
new 2408467-12-stark-after.png3.45 KB
new 2408467-12-stark-rtl-after.png3.57 KB
new 2408467-12-seven-after.png2.08 KB
new 2408467-12-seven-rtl-after.png2.02 KB
new 2408467-12-bartik-after.png14.5 KB
new 2408467-12-bartik-rtl-after.png14.95 KB

I have attached some screenshots with the current looks.

+++ b/core/themes/seven/css/components/pager.css
@@ -16,11 +17,9 @@
-  -webkit-font-smoothing: antialiased;

This change seems to make no visual difference on Windows Google Chrome. Could somebody on a Mac confirm this change is ok?

lewisnyman’s picture

lewisnyman
he/him
commented

This test pages shows a different in rendering: http://maxvoltar.com/sandbox/fontsmoothing/ - Maybe it depends on your install fonts? I'm tempted to keep it

idebr’s picture

idebr commented
Status: Needs review » Needs work

This test pages shows a different in rendering: http://maxvoltar.com/sandbox/fontsmoothing/ - Maybe it depends on your install fonts? I'm tempted to keep it

Yes, that was what I was afraid of. Font smoothing is much more pronounced on Mac computers. 

+++ b/core/themes/seven/css/components/pager.css
@@ -16,11 +17,9 @@
-  -webkit-font-smoothing: antialiased;

Let's keep this line for font smoothing

DickJohnson’s picture

DickJohnson commented
StatusFileSize
new 2408467-14.patch1.77 KB
new 2408467-12-14.txt402 bytes

Put the -webkit-font-smoothing: antialiased; where it used to be.

DickJohnson’s picture

DickJohnson commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: 2408467-14.patch, failed testing.

DickJohnson’s picture

DickJohnson commented
Status: Needs work » Needs review

That doesn't make sense to me, so I'll try again.

DickJohnson queued 16: 2408467-14.patch for re-testing.

idebr’s picture

idebr commented
Status: Needs review » Reviewed & tested by the community

Looking good, thanks @DickJohnson :)

All CSS coding standards issues have been addressed, settings to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

CSS changes are not blocked in beta. Committed 112ed4f and pushed to 8.0.x. Thanks!

  • alexpott committed 112ed4f on 
    Issue #2408467 by idebr, DickJohnson, LewisNyman, monobasic: Rewrite...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.