Follow-up to #1995272: [Meta] Refactor module CSS files inline with our CSS standards

Remaining tasks

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

In this issue, we are choosing to work on csslint and formatting fixes, instead of rewriting every CSS class, as the scope and complexity of the views UI is huge. It would be difficult to change everything in one patch.

User interface changes

None

API changes

None

Views testing process

Views is by far Drupal Core's most complex UI, it requires it's own testing coverage to ensure a commit does not cause regressions. Each issue can provide before/after screenshots of the following use cases.

Simple use cases

  • Add a new view via the wizard (admin/structure/views/add
  • Select the basics and hit save.
  • On the view edit form, select a new filter for the node type
  • Change the row format to show fields
  • Add the body field to the output, and rewrite the output to use
    [body]
  • save the view

Advanced use cases

  • Enable a display
  • Disable a display
  • Choose the table style and enable click sort
  • Add a filter by title
  • Add a filter by author
  • Rearrange the filters to have the author first
  • Add a relationship
  • Override the fields configuration // To test the override select
  • Add a contextual filter for Node: ID and setup a validation for a specific node: type
  • Expose a filter
  • Click preview

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because coding standards
Issue priority Not critical because coding standards
Unfrozen changes Unfrozen because it only changes markup
CommentFileSizeAuthor
#60 interdiff-1591744-56-60.txt1.08 KBMathieuSpil
#60 rewrite_views_ui_css-2408525-60.patch41.76 KBMathieuSpil
#57 Content__Content____Site-Install.png763.74 KBLewisNyman
#56 interdiff-51-56.txt3.14 KBnjbarrett
#56 rewrite_views_ui_css-2408525-56.patch41.93 KBnjbarrett
#54 interdiff-2408525-45-51.txt2.5 KBMathieuSpil
#51 rewrite_views_ui_css-2408525-51.patch42.19 KBnjbarrett
#46 Screen Shot 2015-05-10 at 15.36.37.jpg358.67 KBLewisNyman
#46 Screen Shot 2015-05-10 at 15.36.21.jpg316.06 KBLewisNyman
#46 Screen Shot 2015-05-10 at 15.35.58.jpg300.68 KBLewisNyman
#46 Screen Shot 2015-05-10 at 15.35.37.jpg295.76 KBLewisNyman
#46 Screen Shot 2015-05-10 at 15.31.20.jpg378.72 KBLewisNyman
#46 Screen Shot 2015-05-10 at 15.30.46.jpg400.41 KBLewisNyman
#46 Screen Shot 2015-05-10 at 15.21.58.jpg318.77 KBLewisNyman
#46 Screen Shot 2015-05-10 at 15.21.14.jpg339.36 KBLewisNyman
#46 Screen Shot 2015-05-10 at 15.20.55.jpg500.9 KBLewisNyman
#46 Screen Shot 2015-05-10 at 15.20.52.jpg403.94 KBLewisNyman
#46 Screen Shot 2015-05-10 at 15.20.38.jpg297.64 KBLewisNyman
#46 Screen Shot 2015-05-10 at 15.20.27.jpg278.07 KBLewisNyman
#46 Screen Shot 2015-05-10 at 15.20.19.jpg275.79 KBLewisNyman
#45 2408525_45.patch44.19 KBcosmicdreams
#42 2408525_42.patch41.05 KBcosmicdreams
#41 interdiff.txt695 bytescosmicdreams
#41 2408525_41.patch695 bytescosmicdreams
#40 interdiff.txt1.07 KBcosmicdreams
#40 2408525_40.patch40.97 KBcosmicdreams
#38 Screen Shot 2015-05-08 at 09.37.30.png30.3 KBMathieuSpil
#37 Screen Shot 2015-05-08 at 11.49.01 am.png35.89 KBkamalpreetkaur
#37 Screen Shot 2015-05-08 at 11.44.24 am.png41.74 KBkamalpreetkaur
#36 interdiff-2408525-30-36.txt887 bytesMathieuSpil
#36 refactor-views-ui-css-2408525-36.patch40.52 KBMathieuSpil
#34 interdiff-2408525-30-33.txt745 bytesMathieuSpil
#33 refactor-views-ui-css-2408525-33.patch40.77 KBkamalpreetkaur
#32 Screenshot 2015-05-03 11.22.45.jpg122.73 KBLewisNyman
#32 2408525.backstop.json_.txt640 bytesLewisNyman
#32 2408525-BackstopJS-report.pdf1.05 MBLewisNyman
#30 interdiff-2408525-26-30.txt1.49 KBMathieuSpil
#30 refactor-views-ui-css-2408525-30.patch40.63 KBMathieuSpil
#26 refactor-views-ui-css-2408525-26.patch40.84 KBManjit.Singh
#26 interdiff-2408525-25-26.txt512 bytesManjit.Singh
#26 views-ui-before-applying-patch.png146.87 KBManjit.Singh
#26 views-ui-after-applying-patch.png147.33 KBManjit.Singh
#25 interdiff-2408525-15-25.txt855 bytesMathieuSpil
#25 refactor-views-ui-css-2408525-25.patch40.84 KBMathieuSpil
#23 screenshot--admin-structure-views-viewithcontent_0.diff.png139.25 KBaxe312
#23 screenshot--admin-structure-views-viewithcontent_0.png206.9 KBaxe312
#23 screenshot--admin-structure-views-viewithcontent_0.fail_.png153.09 KBaxe312
#16 refactor-views-ui-css-2408525-15.patch40.56 KBMathieuSpil
#13 refactor-views-ui-css-2408525-13.patch38.04 KBMathieuSpil
#11 Screenshot 2015-04-16 15.36.32.jpg717.19 KBLewisNyman
#11 Screenshot 2015-04-16 15.36.21.jpg164.52 KBLewisNyman
#10 refactor-views-ui-css-2408525-10.patch38.03 KBMathieuSpil
#6 refactor-views-ui-css-2408525-6.patch2.28 KBManjit.Singh
#5 refactor-views-ui-css-2408525-5.patch2.28 KBMathieuSpil
#1 views_ui.admin_.css_.txt5.33 KBLewisNyman
#1 views_ui.admin_.theme_.css_.txt23.83 KBLewisNyman
#1 views_ui.contextual.css_.txt1.38 KBLewisNyman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

LewisNyman’s picture

Issue summary: View changes
LewisNyman’s picture

I will start by reviewing the smallest file :P I don't understand why views is overriding the the contextual links?

  1. /**
     * The .contextual.css file is intended to contain styles that override declarations
     * in the Contextual module.
     */
    

    This needs a @file comment, see: https://www.drupal.org/node/1887862#file-comments

  2. /* @group Wrapper */
    ...
    /* @end */
    ...
     
    ...
    /* @end */
    ...
    /* @group List */
    

    These @group/@end comments aren't part of our CSS standards, so we should remove them. These comments are all one word and don't seem to give much help describing the code.

  3. #views-live-preview .contextual-region-active {
    ...
    #views-live-preview div.contextual {
    ...
    [dir="rtl"] #views-live-preview div.contextual {
    

    We need to replace this id with a class

  4. html.js #views-live-preview div.contextual {
    

    We can use .js instead of html.js

  5. #views-live-preview a.contextual-links-trigger {
    

    We can remove the a element here

  6. div.contextual ul.contextual-links {
    ...
    [dir="rtl"] div.contextual ul.contextual-links {
    ...
    ul.contextual-links li a,
    ul.contextual-links li span {
    ...
    ul.contextual-links li span {
    ...
    ul.contextual-links li a {
    ...
    ul.contextual-links li a:hover {
    

    We don't need the div and ul elements on these selectors

MathieuSpil’s picture

Assigned: Unassigned » MathieuSpil
MathieuSpil’s picture

Assigned: MathieuSpil » Unassigned
Status: Active » Needs work
FileSize
2.28 KB

For views_ui.contextual.css
1-2: Done in patch.
3. Added class .views-live-preview on the #views-live-preview and made I now use the CSS-selector.
4-5: Done in patch
6. Removed unnecessary specificity in selectors
Also, I am in favor of deleting this file because it holds nothing else then subjective tweaks to the contextual links css?

@lewis: Before I do the same changes to the other 2 files, can you verify that adding classes, where ID's are, is a good approach?
I left the Id's intact because it's javascript is based upon it.

Manjit.Singh’s picture

@MathieuSpil removed unnecessary space after every selectors.

MathieuSpil’s picture

Assigned: Unassigned » MathieuSpil
LewisNyman’s picture

@lewis: Before I do the same changes to the other 2 files, can you verify that adding classes, where ID's are, is a good approach?
I left the Id's intact because it's javascript is based upon it.

In those cases we want to replace the ID's with classes that are prefixed with .js-. See: https://www.drupal.org/node/1887918#formatting

MathieuSpil’s picture

Issue tags: +drupaldevdays
MathieuSpil’s picture

Assigned: MathieuSpil » Unassigned
Status: Needs work » Needs review
FileSize
38.03 KB

Refactoring further on patch #6:
General
I replaced all the ID's with classes inside the selectors, while double-checking for specificity issues (For now I didn't delete any ID's from the output, only added the classes.)
views_ui.contextual.css:
Extra removal of a color defenition of the contextual links.

views_ui.admin.theme.css:
This was no longer being used, so removed it:

#edit-options-more {
  clear: both;
}

Replaced every use of the class .secondary with .tabs (Readability)

This was no longer being used, so removed it: (There is no #views-display-top inside .views-display-top)

.views-display-top #views-display-top {
  max-width: 180px;
}

This was no longer being used, so removed it:

#edit-display-settings-title {
  font-size: 14px;
  line-height: 1.5;
  margin: 0;
}

This doesn't do anything because of other margins

#edit-displays-settings-settings-content {
  margin-top: 12px;
}

This is no longer being used:

/* @todo the width and border info could be moved into a more generic class */
/* @todo Make this a class to be used anywhere there's node types? */
.form-type-checkboxes #edit-options-value,
.form-type-checkboxes #edit-options-validate-options-node-types {
  border-color: #ccc;
  border-style: solid;
  border-width: 1px;
  max-height: 210px;
  overflow: auto;
  margin-top: 5px;
  padding: 0 5px;
  width: 190px;
}

This is no longer being used: (Because according to views.view.grid.html.twig, the grid is no longer using th's or td's)

#views-live-preview .views-view-grid th,
#views-live-preview .views-view-grid td {
  vertical-align: top;
}

This was not being used, because .item-list is never a direct child of .view-content

#views-live-preview .view-content > .item-list > ul {
  list-style-position: outside;
  padding-left: 21px; /* LTR */
}
[dir="rtl"] #views-live-preview .view-content > .item-list > ul {
  padding-left: 0;
  padding-right: 21px;
}

These ID's are no longer being used.

#edit-options-default-action {
  width: 300px;
  float: left;
}
#edit-options-exception {
  float: right;
  width: 250px;
  margin-top: -2px;
}

views_ui.admin.css:
Removed, because it doesn't apply to anything (and if it did, it got overridden)

.js .views-ui-display-tab-bucket:first-of-type {
  border-top: none;
}

Removed, because the selectors don't apply anywhere?

.views-ui-dialog #views-ajax-popup {
  padding: 0;
  overflow: hidden;
}
.views-ui-dialog #views-ajax-body {
  margin: 0;
  padding: 0;
}

Some thoughts for follow-up tickets:
Refactoring everything in these 3 files is too huge for one ticket, so I suggest that we do some more refactoring in other tickets. We can see this ticket as a primary refactoring for clarity.
1. Discuss the removal of the views_ui.contextual.css (this file only overwrites the contextual css and there is no reason that should happen.
2. Replace the existing ID's with classes that are prefixed with .js-. See: https://www.drupal.org/node/1887918#formatting (This will need js-refactoring as well as adjusting the tests)
3. Combining the views_ui.admin.theme.css and views_ui.admin.css into one, and re-order the css so the readability improves a lot. (If we combined these 2 files, further refactoring will be more straightforward, and reviewable)

LewisNyman’s picture

Status: Needs review » Needs work
FileSize
164.52 KB
717.19 KB

Nice work! I agree we shoudn't refactor everything here but it would be nice to see if we could remove some csslint errors easily- #1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core

  1. +++ b/core/modules/views_ui/css/views_ui.admin.css
    @@ -88,16 +74,12 @@
    - *
    +/* ¶
    

    There is some accidental whitespace here, also I think in our commenting standards we have /**

  2. +++ b/core/modules/views_ui/css/views_ui.admin.theme.css
    @@ -462,57 +345,45 @@ td.group-title {
    -.views-displays .tabs.secondary li.tabs__tab:hover {
    +.views-displays .tabs li.tabs__tab:hover {
    ...
    -[dir="rtl"] .views-displays .tabs.secondary li.tabs__tab:hover {
    +[dir="rtl"] .views-displays .tabs li.tabs__tab:hover {
    

    I these situations can we reduce selectors to .view-displays .tabs.secondary .tabs__tab remove the li. but I think we still need to specify secondary tabs only

  3. +++ b/core/modules/views_ui/css/views_ui.admin.theme.css
    @@ -521,276 +392,178 @@ td.group-title {
    +.views-displays .tabs a.error {
    

    I think we could remove the a here?

There are a few more CSSlint errors, see the screenshots here

MathieuSpil’s picture

Assigned: Unassigned » MathieuSpil
MathieuSpil’s picture

Patch no longer applied so rerolled without any changes (will use this as a fallback)

LewisNyman’s picture

Status: Needs work » Needs review
njbarrett’s picture

MathieuSpil’s picture

1. Removed trailing whitespace

2. I was not aware of the multilangual tabs in views. But after some thoughts: .tabs doesn't needs the .secondary-class because the selector goes as follows: .views-displays .tabs the language/primary tabs wouldn't be selected either. Also the naming of secondary tabs, when there aren't any primary tabs is not very readable.

3. Removed the a-selector here

Also fixed the CSS lint errors (I suppose any further css-refactoring is for follow-up tickets?)
views_ui.admin.css

4. Unknown property 'padding-start'. Properties should be known (listed in CSS3 specification) or be a vendor-prefixed property. (known-properties)

.views-admin ul,
.views-admin menu,
.views-admin dir {
  padding-left: 0; /* LTR for IE */
  /* padding-start is used so that RTL works out of the box */
  -moz-padding-start: 0;
  -webkit-padding-start: 0;
  padding-start: 0;
}

changed to

.views-admin ul,
.views-admin menu,
.views-admin dir {
  padding: 0;
}

Because padding is always set to 0 (if it is at the start or at the end.)

5. float can't be used with display: inline-block. Certain properties shouldn't be used with certain display property values. (display-property-grouping)
Removed inline-block declaration inside:

.views-displays .tabs > li {
  border-right: 0 none; /* LTR */
  display: inline-block;
  float: left; /* LTR */
  padding: 0;
}

6. Element (li.add) is overqualified, just use .add without element name. Don't use classes or IDs with elements (a.foo or a#foo). (overqualified-elements)
changed:

.views-displays .tabs li.add {
  position: relative;
}

to

.views-displays .tabs .add {
  position: relative;
}

7. Element (details.collapsed) is overqualified, just use .collapsed without element name. Don't use classes or IDs with elements (a.foo or a#foo). (overqualified-elements)
I removed the

.js .views-display-column details.collapsed {
  height: auto;
}

Because the details-containers are no longer using the 'collapsed class'

8. Float can't be used with display: inline-block. Certain properties shouldn't be used with certain display property values. (display-property-grouping)
Removed display: inline-block; out of

.views-display-setting .label,
.views-display-setting .views-ajax-link {
  display: inline-block;
  float: left; /* LTR */
}

9. Element (div.form-item-options-value-all) is overqualified, just use .form-item-options-value-all without element name. Don't use classes or IDs with elements (a.foo or a#foo). (overqualified-elements)
Removed the div out of

.form-item-options-value-all {
  display: none;
}

views_ui.admin.theme.css
10. Background image '../images/sprites.png' was used multiple times, first declared at line 28, col 3. Every background-image should be unique. Use a common class for e.g. sprites. (duplicate-background-images)

11. Background image '../images/sprites.png' was used multiple times, first declared at line 28, col 3. Every background-image should be unique. Use a common class for e.g. sprites. (duplicate-background-images)
I want to keep it for now, because the multiple declaration is because of a sprite (this should go into a follow-up ticket)

12. Expected end of value but found 'repeat-y'. Properties should be known (listed in CSS3 specification) or be a vendor-prefixed property. (known-properties)
Same as 11(above), because it states the same background-image

13. Float can't be used with display: inline-block. Certain properties shouldn't be used with certain display property values. (display-property-grouping)
Removed display: inline-block; out of

.views-admin span.icon {
  display: inline-block;
  float: left; /* LTR */
  position: relative;
}

14. Negative text-indent doesn't work well with RTL. If you use text-indent for image replacement explicitly set direction for that item to ltr. Checks for text indent less than -99px (text-indent)
Added direction: ltr; to

.views-admin .icon.compact {
  display: block;
  overflow: hidden;
  text-indent: -9999px;
}

15 + 16. Element (input.form-checkbox) is overqualified, just use .form-checkbox without element name. Don't use classes or IDs with elements (a.foo or a#foo). (overqualified-elements) Element (input.form-radio) is overqualified, just use .form-radio without element name. Don't use classes or IDs with elements (a.foo or a#foo). (overqualified-elements)
removed the element name input from:

input.form-checkbox,
input.form-radio {
  vertical-align: baseline;
}

17 + 18. Heading (h1) should not be qualified. Headings should not be qualified (namespaced). (qualified-headings) Element (h1.unit-title) is overqualified, just use .unit-title without element name. Don't use classes or IDs with elements (a.foo or a#foo). (overqualified-elements)
Removed h1 out of:

.views-admin h1.unit-title {
  font-size: 15px;
  line-height: 1.6154;
  margin-bottom: 0;
  margin-top: 18px;
}

19-23 Element (th.views-ui-name) is overqualified, just use .views-ui-name without element name. Don't use classes or IDs with elements (a.foo or a#foo). (overqualified-elements)
Removed the th out of these selectors:

th.views-ui-name {
  width: 18%;
}
th.views-ui-description {
  width: 26%;
}
th.views-ui-tag {
  width: 8%;
}
th.views-ui-path {
  width: auto;
}
th.views-ui-operations {
  width: 24%;
}

24. Outlines shouldn't be hidden unless other visual changes are made. Use of outline: none or outline: 0 should be limited to :focus rules. (outline-none)
Added text-decoration: underline, so that this passes the csslint test

25. Float can't be used with display: inline-block. Certain properties shouldn't be used with certain display property values. (display-property-grouping)
Fixed by removing display: inline-block

26-30. Heading (h3) should not be qualified. Headings should not be qualified (namespaced). (qualified-headings)
Added views-ui-display-tab-bucket__title as a class to the bucket title (name should change in later refactoring round)

31-33 Unqualified attribute selectors are known to be slow. Unqualified attribute selectors are known to be slow. (unqualified-attributes)
the [data-drupal-views-offset] selectors have now been replaced with .views-offset selectors

34. Heading (h2) should not be qualified. Headings should not be qualified (namespaced). (qualified-headings)
Can someone point me out, where I can find this element: .views-ui-dialog .views-ajax-title h2

35. Element (tr[id^="views-row"].even) is overqualified, just use .even without element name. Don't use classes or IDs with elements (a.foo or a#foo). (overqualified-elements)
Removed tr[id^="views-row"] out of

.views-ui-rearrange-filter-form tr[id^="views-row"].even td {
  background-color: #f3f4ed;
}

36. Element (div.messages) is overqualified, just use .messages without element name. Don't use classes or IDs with elements (a.foo or a#foo). (overqualified-elements)
Removed following code because the default messages styling should not be messed around with:

div.messages {
  margin-bottom: 18px;
  line-height: 1.4555;
}

I think the most robust changes have now been made to these files, and after finishing these, we should create a bunch of smaller tickets to tackle each remaining refactoring-type like reorganisation on file-level. Refactoring inside each css-file, more simplifying the css and then think about how we can improve the ux of a whole bunch of elements. But for sanity's sake we should probably stop any further refactoring inside this ticket.

MathieuSpil’s picture

Assigned: MathieuSpil » Unassigned

Status: Needs review » Needs work

The last submitted patch, 16: refactor-views-ui-css-2408525-15.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: refactor-views-ui-css-2408525-15.patch, failed testing.

Status: Needs work » Needs review
LewisNyman’s picture

Issue tags: +Needs screenshots

Great, now it is passing we need some screenshots to show that we haven't caused any regressions.

axe312’s picture

I am sorry, but I found some visual regressions. Looks like the "Filter" button is missing a "clear: left;" or sth like this. Screenshots are attached.

MathieuSpil’s picture

Assigned: Unassigned » MathieuSpil
MathieuSpil’s picture

Assigned: MathieuSpil » Unassigned
Status: Needs work » Needs review
FileSize
40.84 KB
855 bytes

Ah yes, good catch.

Added a class to the preview submit button so I could refactor a bit more straight-forward according to the preview button.

Manjit.Singh’s picture

@MathieuSpil removing one minor whitespace from views_ui.admin.theme.css in your patch.

Also found some UI issues, Please check screenshots.

LewisNyman’s picture

Issue summary: View changes
LewisNyman’s picture

Status: Needs review » Needs work

Setting to needs work based on #26

MathieuSpil’s picture

Assigned: Unassigned » MathieuSpil

Thanks for testing Manjit!

Looking into this.

MathieuSpil’s picture

Status: Needs work » Needs review
FileSize
40.63 KB
1.49 KB

Worked further on #26:
I didn't look at the css-specificity of tabs.css when refactoring, so resetted some lines from my earlier patch (removal of the .secondary-class)

So if someone could provide screenshots of not only the general page but also of the modal-popups and the general page at admin/structure/views/ that would be great.

Please also enable the 4 internationalisation modules so the primary tabs are also visible in the header.

MathieuSpil’s picture

Assigned: MathieuSpil » Unassigned
LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
1.05 MB
640 bytes
122.73 KB

I ran visual regression tests on the pages that are accessible from urls and I've attached the PDF and the config I used. Looks like we have some spacing issues for the secondary tabs. We still have to do some manual testing on the steps that can't be accessed by URL. I've added some steps to the issue summary.

kamalpreetkaur’s picture

Thanks Lewis for the tests. I further worked on #30 and made changes for the spacing and margin of Save and Cancel Buttons. Please help with regression tests on the issues

MathieuSpil’s picture

Status: Needs work » Needs review
FileSize
745 bytes

Placing on review to trigger testbot + attached interdiff for clarity's sake.

MathieuSpil’s picture

Status: Needs review » Needs work

Patch #33 causes new regressions on top op #30.
Can we for now focus only on the visual regression in the tab-menu?

Placing back to needs work

EDIT: Hmmm, it seems that I didn't properly reviewed the BackstopJS report of Lewis. My bad.

MathieuSpil’s picture

Status: Needs work » Needs review
FileSize
40.52 KB
887 bytes

@kamalpreetkaur: Thanks for looking in to this, I think the best approach is to reset changes from the large patch (in #30) instead of writing extra css to cover up my initial mistakes :)

Worked further and resetted some of my changes.

The visual regression in the tabs navigation was a intentional change (the buttons were not vertically centered initially and on mobile they all stuck to eachother). But I would love to hear from others if this is a improvement...

kamalpreetkaur’s picture

Hi MathieuSpil, yeah reset is the best thing. But, I was working on the Save and Cancel buttons-spacing issue on mobile. I also worked on the Preview form text-box and
Update preview button as it was hitting the boundary.

Please see the attached screenshot of #33 and #36

MathieuSpil’s picture

Ah ok, I see where you are coming from.
After the initial cleaning up inside this issue, I will create a bunch of follow-up tickets for all the other bugs that are now inside views.
I will make sure that the button issue is also one of those tickets :)

The reason why is because the css inside views is so chaotic, it is very hard to actual make changes to the look en feel of it.

Without any patch, the views already looks like this:

cosmicdreams’s picture

Hi all, I'm looking into this issue as it looks like a great way to make a impact for Drupal's css.

With the patch in #36 applied there are still 5 errors in views_ui.admin.theme.css. they are:

Line Column Message
37 3 Background image '../images/sprites.png' was used multiple times, first declared at line 28, col 3.
46 3 Background image '../images/sprites.png' was used multiple times, first declared at line 28, col 3.
52 3 Expected end of value but found 'repeat-y'.
645 36 Heading (h2) should not be qualified.
781 1 Element (div.messages) is overqualified, just use .messages without element name.

While the issues with the button are being looked at by others, I'll turn my attention to these.

cosmicdreams’s picture

FileSize
40.97 KB
1.07 KB

Here's a patch that resolves all but 1 lint error for views_ui.admin.theme.css. The last remaining error is due to the h2 that is injected to to ajax enabled view's titles.

If we can ensure that this h2 has a class we could use it as a selector instead of the overly general h2.

Note: CSS selectivity works right-to-left (not left-to-right like I thought it did before a frontend developer pointed this out to me). So not using the overly general h2 to select the title element does matter.

cosmicdreams’s picture

FileSize
695 bytes
695 bytes

Had a good chat with Lewis Nyman about the issues we're seeing with the button. I removed the styles that were adding the padding for LTR and RTL displays of the button. We will rely on Seven's styles to handle any additional positioning of the button that needs to happen. Views admin's css isn't messing with this button anymore

cosmicdreams’s picture

FileSize
41.05 KB

Oops wrong patch, this is the right one.

LewisNyman’s picture

Status: Needs review » Needs work
Issue tags: +csslint

If we can ensure that this h2 has a class we could use it as a selector instead of the overly general h2.

It would be great if we could add a class instead of using the element, it makes it a little less fragile if someone changed the heading element. Can we move the .views-ajax-title class onto the h2?

cosmicdreams’s picture

Assigned: Unassigned » cosmicdreams

Looking into it.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
44.19 KB

Was able to talk with dawhener of the Views team and discovered that this style refers to a views modal dialog component that is no longer used. The real fix is to not only remove the CSS but also the php that places this dormant and unused markup on the page.

LewisNyman’s picture

Title: Rewrite Views UI CSS inline with our CSS standards » Rewrite Views UI CSS inline with our CSS standards - Part 1
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots
FileSize
275.79 KB
278.07 KB
297.64 KB
403.94 KB
500.9 KB
339.36 KB
318.77 KB
400.41 KB
378.72 KB
295.76 KB
300.68 KB
316.06 KB
358.67 KB

OK so I went through all the test steps posted in the issue summary and took screenshots. I didn't find any regressions! Amazing!

Thanks for the work. I've added an explanation why we are not tackling all the CSS problems in one issue.

cosmicdreams’s picture

Oh wow, thanks for saving me the time of creating these.

cosmicdreams’s picture

Oh wow, thanks for saving me the time of creating these.

LewisNyman’s picture

Status: Reviewed & tested by the community » Needs work
--- a/core/lib/Drupal/Core/Entity/DependencyTrait.php
+++ b/core/lib/Drupal/Core/Entity/DependencyTrait.php

@@ -23,11 +23,10 @@
-   *   Type of dependency being added: 'module', 'theme', 'config', 'content'.
+   *   The type of dependency being added: 'module', 'theme', or 'entity'.
...
-   *   $type is 'config' or 'content', the result of
-   *   EntityInterface::getConfigDependencyName().
+   *   $type is 'entity', the full configuration object name.

@@ -664,24 +664,6 @@ function system_requirements($phase) {
-  // Check xdebug.max_nesting_level, as some pages will not work if it is too
-  // low.
-  if (extension_loaded('xdebug')) {
-    // Setting this value to 256 was considered adequate on Xdebug 2.3
-    // (see http://bugs.xdebug.org/bug_view_page.php?bug_id=00001100)
-    $minimum_nesting_level = 256;
-    $current_nesting_level = ini_get('xdebug.max_nesting_level');
-
-    if ($current_nesting_level < $minimum_nesting_level) {
-      $requirements['xdebug_max_nesting_level'] = [
-        'title' => t('Xdebug settings'),
-        'value' => t('xdebug.max_nesting_level is set to %value.', ['%value' => $current_nesting_level]),
-        'description' => t('Set <code>xdebug.max_nesting_level=@level

in your PHP configuration as some pages in your Drupal site will not work when this setting is too low.', ['@level' => $minimum_nesting_level]),
- 'severity' => REQUIREMENT_ERROR,
- ];
- }
- }
-

Looks like there are some changes from another patch here.

LewisNyman’s picture

Issue tags: +Novice

It's a novice task to apply the patch, remove the unintentional changes, and then upload the new patch.

njbarrett’s picture

Assigned: cosmicdreams » Unassigned
FileSize
42.19 KB

New patch attached. Rerolled and removed unintended changes from previous patch.

njbarrett’s picture

Status: Needs work » Needs review

The last submitted patch, 33: refactor-views-ui-css-2408525-33.patch, failed testing.

MathieuSpil’s picture

Not following this context entirely but attached interdiff for clarity's sake.

LewisNyman’s picture

Status: Needs review » Needs work
Issue tags: -Novice
  1. +++ b/core/modules/views_ui/css/views_ui.admin.theme.css
    @@ -798,136 +546,95 @@ td.group-title {
    -.views-ui-dialog #views-ajax-title,
     .views-ui-dialog .views-override {
    
    +++ b/core/modules/views_ui/src/ViewEditForm.php
    @@ -122,13 +122,6 @@ public function form(array $form, FormStateInterface $form_state) {
    -    $form['#attached']['drupalSettings']['views']['ajax'] = [
    -      'id' => '#views-ajax-body',
    -      'title' => '#views-ajax-title',
    -      'popup' => '#views-ajax-popup',
    -      'defaultForm' => $view->getDefaultAJAXMessage(),
    -    ];
    
    @@ -218,19 +214,6 @@ public function form(array $form, FormStateInterface $form_state) {
    -      // The content of the popup dialog.
    -      $form['ajax-area'] = array(
    -        '#type' => 'container',
    -        '#id' => 'views-ajax-popup',
    -      );
    -      $form['ajax-area']['ajax-title'] = array(
    -        '#markup' => '<div id="views-ajax-title"></div>',
    -      );
    -      $form['ajax-area']['ajax-body'] = array(
    -        '#type' => 'container',
    -        '#id' => 'views-ajax-body',
    -      );
    

    Looks like we are removing these IDs and replacing them in the CSS with existing classes. How about for now we just replace the ID's with classes? Something like 'title' has more meaning than 'override'

  2. +++ b/core/modules/views_ui/src/ViewUI.php
    @@ -198,10 +198,6 @@ public function set($property_name, $value, $notify = TRUE) {
     
    -  public static function getDefaultAJAXMessage() {
    -    return SafeMarkup::set('<div class="message">' . t("Click on an item to edit that item's details.") . '</div>');
    -  }
    -
    

    It looks like we are removing this function when we shouldn't. This was probably added in another issue.

njbarrett’s picture

Status: Needs work » Needs review
FileSize
41.93 KB
3.14 KB

@Lewis hopefully I understood you correctly - here's an updated patch and interdiff

LewisNyman’s picture

Status: Needs review » Needs work
FileSize
763.74 KB

@njbarrett Thanks, it seems that the classes don't match up like I imagined, it looks like the correct class we should use instead of .views-ajax-title is .views-offset-top? It looks like after that we are done.

MathieuSpil’s picture

Assigned: Unassigned » MathieuSpil
MathieuSpil’s picture

Getting back on this during Frontend United.

MathieuSpil’s picture

Assigned: MathieuSpil » Unassigned
Status: Needs work » Needs review
FileSize
41.76 KB
1.08 KB

So I first resetted the CSS-changes from #56 and double checked that visually it looks ok again, then I spotted a couple of very clear things that are not needed in the css (very tiny changes).

@LewisNyman:
Do we still need the html-changes suggested in #56? (got lost in the last 10 comments here)

And thanks @njbarrett for diving into this chaos!

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

@MathieuSpil I think we're ok using the views-override class in issue, we can rename classes so they make more sense in cleanup part 2! Thanks for the hard work,

dawehner’s picture

  1. +++ b/core/modules/views_ui/admin.inc
    @@ -223,7 +223,7 @@ function views_ui_standard_display_dropdown(&$form, FormStateInterface $form_sta
    -    '#prefix' => '<div class="views-override clearfix form--inline" data-drupal-views-offset="top">',
    +    '#prefix' => '<div class="views-override clearfix form--inline views-offset-top" data-drupal-views-offset="top">',
    
    @@ -231,7 +231,7 @@ function views_ui_standard_display_dropdown(&$form, FormStateInterface $form_sta
    -    $form['progress']['#markup'] = '<div id="views-progress-indicator">' . t('@current of @total', array('@current' => $form_progress['current'], '@total' => $form_progress['total'])) . '</div>';
    +    $form['progress']['#markup'] = '<div id="views-progress-indicator" class="views-progress-indicator">' . t('@current of @total', array('@current' => $form_progress['current'], '@total' => $form_progress['total'])) . '</div>';
    

    Mh, why do we need new selectors even we already have some?

  2. +++ b/core/modules/views_ui/css/views_ui.admin.theme.css
    @@ -798,136 +546,90 @@ td.group-title {
    -
    -.views-ui-dialog #views-ajax-title h2 {
    -  font-size: 15px;
    -  padding: 8px 13px;
    -  margin: 0;
    -}
    -
    
    @@ -935,38 +637,14 @@ td.group-title {
    -/* @todo the width and border info could be moved into a more generic class */
    -/* @todo Make this a class to be used anywhere there's node types? */
    -.form-type-checkboxes #edit-options-value,
    -.form-type-checkboxes #edit-options-validate-options-node-types {
    -  border-color: #ccc;
    -  border-style: solid;
    -  border-width: 1px;
    -  max-height: 210px;
    -  overflow: auto;
    -  margin-top: 5px;
    -  padding: 0 5px;
    -  width: 190px;
    -}
    

    Its a little bit odd to remove CSS without an obvious replacement ... ?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 60: rewrite_views_ui_css-2408525-60.patch, failed testing.

MathieuSpil’s picture

Is #2502255: Enable tags cache plugin by default for Views related to the failing test?
Otherwise I don't know what the errors mean. (3 days ago, it passed)

The last submitted patch, 60: rewrite_views_ui_css-2408525-60.patch, failed testing.

LewisNyman’s picture

Status: Needs work » Needs review

Ok it's passing, I'll try and find some spare time to respond to @dawehner's comments. This is a big patch so I need a good chunk of time :)

LewisNyman’s picture

Mh, why do we need new selectors even we already have some?

The views-offset-bottom views-offset-top were added to replace the [data-views-offset] selectors as recommended by CSSlint, see points 31-33 in #16.

Its a little bit odd to remove CSS without an obvious replacement ... ?

See this comment in #10:

This is no longer being used: (Because according to views.view.grid.html.twig, the grid is no longer using th's or td's)

I think there is a lot more CSS that can be removed, but let's not bite off too much in one patch. It's far from perfect but it's a good start, we've removed a lot of CSSlint errors from core :)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I think its wrong to blow up our markup for the non-common usecase, but well, this is what banana was about.

LewisNyman’s picture

@dawehner Don't worry we will trim your markup right down when we get the chance ;-)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Being blunt this is probably one of those issues it is best to commit and see what falls out. Thanks @dawehner for reviewing.

Committed ac521b9 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed ac521b9 on 8.0.x
    Issue #2408525 by MathieuSpil, cosmicdreams, Manjit.Singh, njbarrett,...

Status: Fixed » Closed (fixed)

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

tim.plunkett’s picture

+++ b/core/modules/views_ui/css/views_ui.admin.theme.css
@@ -47,54 +33,30 @@
-  background-image:
-    url(../images/sprites.png),
-    -webkit-gradient(
-      linear,
-      left top,
-      left bottom,
-      color-stop(0.0, rgba(255, 255, 255, 1.0)),
-      color-stop(1.0, rgba(232, 232, 232, 1.0))
-    );
-  background-image:
-    url(../images/sprites.png),
-    -webkit-linear-gradient(
-      -90deg,
-      #fff 0,
-      #e8e8e8 100%);
-  background-repeat: no-repeat, repeat-y;
+  background: linear-gradient(-90deg, #fff 0, #e8e8e8 100%) no-repeat, repeat-y;

This change was incorrect, and visually looks silly. Why was this done?

joelpittet’s picture

From what I can tell it maybe was a mistake, but further to that I'm not sure we are using sprites as most of those have been converted to SVG for mobile/retina and that.

Maybe this needs a follow-up to add the SVG equivalents back in?