See: #2405553: Ensure code is inline with the current standards

Remaining tasks

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

User interface changes

None

API changes

None

CommentFileSizeAuthor
#8 Screenshot 2015-03-15 13.22.09.jpg238.29 KBlewisnyman
#8 Screenshot 2015-03-15 13.22.07.jpg253.28 KBlewisnyman
#6 interdiff.txt734 bytespsebborn
#6 clean_up_page_title-2408461-6.patch927 bytespsebborn
#3 clean_up_page-title_2408461_3.patch403 bytespsebborn
#1 page-title.css_.txt250 byteslewisnyman

Comments

lewisnyman’s picture

lewisnyman
he/him
commented
StatusFileSize
new page-title.css_.txt250 bytes

Uploaded the css file so it can reviewed with Dreditor

lewisnyman’s picture

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

psebborn commented
Status: Active » Needs review
StatusFileSize
new clean_up_page-title_2408461_3.patch403 bytes

I went to look at this, and noticed that the CSS posted has changed, as part of #2422351.

I've reviewed the CSS, and really there were two things I changed:

  • addition (of the @file).
  • I've also removed the .content-header from the .page-title selector

I know <div class="page-title"> used elsewhere e.g. maintenance-page.html.twig, but then it's also overriden in mainentance-page.css so it shouldn't be an issue?
Have attached a patch with these two changes in..

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Needs work

Nice thanks. I think there are a few more things we can do here, also I think we can merge some of the styling in the h1 styling as h1 only get's used in the same place as page-title. We will have to double check that we don't cause regression in the install screen.

  1.   color: #333;

    We can remove this color setting as it is set globally. 

  2.   font-size: 1.625em;
  line-height: 1.875em;
  font-weight: 600;
  -webkit-font-smoothing: antialiased;

    Instead of overridden these properties here, can we merge these settings into the h1 styling?

psebborn’s picture

psebborn commented

Hmm, yes that could definitely be merged a bit.

I might be opening a can of worms here, but I'm not sure about the line-height.. To me it's more of a general issue with the theme. On the body the line-height is set with a unit:

font: normal 81.3%/1.538em "Lucida Grande", "Lucida Sans Unicode", "DejaVu Sans", "Lucida Sans", sans-serif;

..what's the rationale for that? The upshot is, for .page-title the font-size is 21px but in Chrome the line-height is 20px. Theoretically it should be relative to the font size but Chrome doesn't behave that way (not for me anyway)

If it was set without a unit , e.g. 1.5 then it would be a nice ratio and could be a set-once, work-everywhere solution..

psebborn’s picture

psebborn commented
StatusFileSize
new clean_up_page_title-2408461-6.patch927 bytes
new interdiff.txt734 bytes

If I ignore that for the context of this issue (as I think it could lead to a whole heap of regression!), then I could propose the attached patch perhaps..?

lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs work » Needs review
lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screenshot 2015-03-15 13.22.07.jpg253.28 KB
new Screenshot 2015-03-15 13.22.09.jpg238.29 KB

Thanks! This makes a lot of sense. Here are some before/after screenshots.

Before:
before patch

After:
after patch

  • alexpott committed 6f83b4e on 8.0.x 
    Issue #2408461 by psebborn: Clean up 'page-title' component
alexpott’s picture

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

CSS is not frozen. Committed 6f83b4e and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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