Input fields box-sizing setting is declared at line 797.

This is duplicated in the "Improve form element usability on narrow devices." section at line 855.

Than those fields are listed again at line 874.

CommentFileSizeAuthor
#11 duplicated-code-seven-styles-css-2202999-11.patch1.41 KBthamas
#11 d8-input-field-box-sizing-not-duplicated.png171.9 KBthamas
#11 d8-input-field-box-sizing-duplicated.png179.27 KBthamas
#1 drupal-duplicated-code-seven-styles-css-2202999-1.patch1.35 KBthamas
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thamas’s picture

thamas
Primary language Hungarian
Location Hungary
CreditAttribution: thamas commented
Issue summary: View changes
Status: Active » Needs review
FileSize
drupal-duplicated-code-seven-styles-css-2202999-1.patch1.35 KB

Remove duplicated (input related) lines, move one line to the right place (max-width) and move the input width setting to the code block which we keep.

(I do not understand the reason of mixing mobile first and desktop first approach in the style.css file, but ironing out that would need more work. Or could someone explain the reasons?)

Cameron Tod’s picture

Cameron Tod CreditAttribution: Cameron Tod commented
Status: Needs review » Reviewed & tested by the community
YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: +Novice, +Needs screenshots

Before and after screenshots (showing it's the same before and after) would help confidence in this review https://drupal.org/contributor-tasks/add-screenshots
Maybe also both wide screen and narrow.

The question in #1 might need an answer about mixing mobile and desktop CSS, and/or a link to an issue trying to discuss/fix that or a follow up issue for it.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs review
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs review » Needs work
Issue tags: -Needs screenshots

I didn't think I'd find a problem but I did ;-)

+++ b/core/themes/seven/style.css
@@ -852,22 +852,6 @@ select.form-select:focus {
   input.form-number {
     width: auto;
   }

This selector needs to be moved down below the one after it otherwise it won't override.

Cameron Tod’s picture

Cameron Tod CreditAttribution: Cameron Tod commented
Issue tags: +DrupalCampLDN
YesCT’s picture

YesCT CreditAttribution: YesCT commented
Assigned: thamas » Unassigned

unassigning... but @thamas if you want to work on that fix, just make a comment. :)

after that change is in, it will need new before and after screenshots.

thamas’s picture

thamas
Primary language Hungarian
Location Hungary
CreditAttribution: thamas commented
Assigned: Unassigned » thamas

I can do it, just had no time. Maybe the next weekend.

I found this when looked an other bug of the installer page (#2202973: Input fields overflow on install config page on desktop), but that is not the best place to take screenshots(?). Perhaps you have a better idea…

Sill’s picture

Sill CreditAttribution: Sill commented

Before Widescreen - http://screencast.com/t/ASNXNt6rJb
Before Narrow - http://screencast.com/t/vzuBe4pH

Applied patch

After Widescreen - http://screencast.com/t/GvPEZ4ERvjVH
After Narrow - http://screencast.com/t/GvPEZ4ERvjVH

- First time contributing to core so hopefully this is useful --

YesCT’s picture

YesCT CreditAttribution: YesCT commented

@Sill Thanks!

it is best to embed images in comments (or the issue summary).
To do that, you will need to upload the files do d.o
https://drupal.org/contributor-tasks/add-screenshots has more help for you.

It would also be good to mark up the screenshots to point out the differences (or note if you saw *no* differences) between before and after.
And also what was expected (are we expecting a difference, or expecting them to be identical?)

How did you decide to use the create article as the page to test?

We should wait for a new patch addressing the concern in #5 before we do another set of screenshots.

thamas’s picture

thamas
Primary language Hungarian
Location Hungary
CreditAttribution: thamas commented
Status: Needs work » Needs review
FileSize
d8-input-field-box-sizing-duplicated.png179.27 KB
d8-input-field-box-sizing-not-duplicated.png171.9 KB
duplicated-code-seven-styles-css-2202999-11.patch1.41 KB

So here we go again with a new patch. Changes are the same as in #1 plus the fix which was asked in #5 (by the way I do not understand why we want auto width to number fields when every other input is 100% width…).

You should see on the screenshots that the field looks the same, but after the change we have less lines of code and no overrided same box-sizing property. :)

Before:

After:

So much rounds from us to make a stylesheet 15 lines lighter… :)

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented

11: duplicated-code-seven-styles-css-2202999-11.patch queued for re-testing.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs review » Reviewed & tested by the community

Ok the issue I found is fixed and this looks good. That patch applies so I'm assuming this comes back green.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit fa84283 on 8.x by catch: 
    Issue #2202999 by thamas: Duplicated lines in style.css of Seven theme.

Status: Fixed » Closed (fixed)

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