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
Issue category | Task because coding standards |
---|---|
Issue priority | Not critical because coding standards |
Unfrozen changes | Unfrozen because it only changes markup |
Comment | File | Size | Author |
---|---|---|---|
#60 | rewrite_views_ui_css-2408525-60.patch | 41.76 KB | MathieuSpil |
#46 | Screen Shot 2015-05-10 at 15.36.21.jpg | 316.06 KB | LewisNyman |
#46 | Screen Shot 2015-05-10 at 15.35.58.jpg | 300.68 KB | LewisNyman |
#46 | Screen Shot 2015-05-10 at 15.35.37.jpg | 295.76 KB | LewisNyman |
#46 | Screen Shot 2015-05-10 at 15.31.20.jpg | 378.72 KB | LewisNyman |
Comments
Comment #1
LewisNyman CreditAttribution: LewisNyman commentedComment #2
LewisNyman CreditAttribution: LewisNyman commentedComment #3
LewisNyman CreditAttribution: LewisNyman at Wunder commentedI will start by reviewing the smallest file :P I don't understand why views is overriding the the contextual links?
This needs a @file comment, see: https://www.drupal.org/node/1887862#file-comments
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.
We need to replace this id with a class
We can use .js instead of html.js
We can remove the a element here
We don't need the div and ul elements on these selectors
Comment #4
MathieuSpil CreditAttribution: MathieuSpil commentedComment #5
MathieuSpil CreditAttribution: MathieuSpil commentedFor 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.
Comment #6
Manjit.Singh@MathieuSpil removed unnecessary space after every selectors.
Comment #7
MathieuSpil CreditAttribution: MathieuSpil commentedComment #8
LewisNyman CreditAttribution: LewisNyman at Wunder commentedIn those cases we want to replace the ID's with classes that are prefixed with
.js-
. See: https://www.drupal.org/node/1887918#formattingComment #9
MathieuSpil CreditAttribution: MathieuSpil commentedComment #10
MathieuSpil CreditAttribution: MathieuSpil commentedRefactoring 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:
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)
This was no longer being used, so removed it:
This doesn't do anything because of other margins
This is no longer being used:
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)
This was not being used, because .item-list is never a direct child of .view-content
These ID's are no longer being used.
views_ui.admin.css:
Removed, because it doesn't apply to anything (and if it did, it got overridden)
Removed, because the selectors don't apply anywhere?
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)
Comment #11
LewisNyman CreditAttribution: LewisNyman at Wunder commentedNice 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
There is some accidental whitespace here, also I think in our commenting standards we have /**
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
I think we could remove the a here?
There are a few more CSSlint errors, see the screenshots here
Comment #12
MathieuSpil CreditAttribution: MathieuSpil commentedComment #13
MathieuSpil CreditAttribution: MathieuSpil commentedPatch no longer applied so rerolled without any changes (will use this as a fallback)
Comment #14
LewisNyman CreditAttribution: LewisNyman at Wunder commentedComment #15
njbarrett CreditAttribution: njbarrett commentedComment #16
MathieuSpil CreditAttribution: MathieuSpil commented1. 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 hereAlso 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)
changed to
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:
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:
to
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
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 of9. 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 ofviews_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 of14. 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;
to15 + 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: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:
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:
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
selectors34. 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 of36. 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:
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.
Comment #17
MathieuSpil CreditAttribution: MathieuSpil commentedComment #22
LewisNyman CreditAttribution: LewisNyman at Wunder commentedGreat, now it is passing we need some screenshots to show that we haven't caused any regressions.
Comment #23
axe312 CreditAttribution: axe312 at Wunder commentedI 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.
Comment #24
MathieuSpil CreditAttribution: MathieuSpil commentedComment #25
MathieuSpil CreditAttribution: MathieuSpil commentedAh 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.
Comment #26
Manjit.Singh@MathieuSpil removing one minor whitespace from
views_ui.admin.theme.css
in your patch.Also found some UI issues, Please check screenshots.
Comment #27
LewisNyman CreditAttribution: LewisNyman at Wunder commentedComment #28
LewisNyman CreditAttribution: LewisNyman at Wunder commentedSetting to needs work based on #26
Comment #29
MathieuSpil CreditAttribution: MathieuSpil commentedThanks for testing Manjit!
Looking into this.
Comment #30
MathieuSpil CreditAttribution: MathieuSpil commentedWorked 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.
Comment #31
MathieuSpil CreditAttribution: MathieuSpil commentedComment #32
LewisNyman CreditAttribution: LewisNyman at Wunder commentedI 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.
Comment #33
kamalpreetkaur CreditAttribution: kamalpreetkaur commentedThanks 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
Comment #34
MathieuSpil CreditAttribution: MathieuSpil commentedPlacing on review to trigger testbot + attached interdiff for clarity's sake.
Comment #35
MathieuSpil CreditAttribution: MathieuSpil commentedPatch #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.
Comment #36
MathieuSpil CreditAttribution: MathieuSpil commented@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...
Comment #37
kamalpreetkaur CreditAttribution: kamalpreetkaur commentedHi 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
Comment #38
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedAh 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:
Comment #39
cosmicdreams CreditAttribution: cosmicdreams commentedHi 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.
Comment #40
cosmicdreams CreditAttribution: cosmicdreams commentedHere'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.
Comment #41
cosmicdreams CreditAttribution: cosmicdreams commentedHad 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
Comment #42
cosmicdreams CreditAttribution: cosmicdreams commentedOops wrong patch, this is the right one.
Comment #43
LewisNyman CreditAttribution: LewisNyman at Wunder commentedIt 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?
Comment #44
cosmicdreams CreditAttribution: cosmicdreams commentedLooking into it.
Comment #45
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedWas 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.
Comment #46
LewisNyman CreditAttribution: LewisNyman at Wunder commentedOK 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.
Comment #47
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedOh wow, thanks for saving me the time of creating these.
Comment #48
cosmicdreams CreditAttribution: cosmicdreams as a volunteer commentedOh wow, thanks for saving me the time of creating these.
Comment #49
LewisNyman CreditAttribution: LewisNyman at Wunder commentedin 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.
Comment #50
LewisNyman CreditAttribution: LewisNyman at Wunder commentedIt's a novice task to apply the patch, remove the unintentional changes, and then upload the new patch.
Comment #51
njbarrett CreditAttribution: njbarrett as a volunteer commentedNew patch attached. Rerolled and removed unintended changes from previous patch.
Comment #52
njbarrett CreditAttribution: njbarrett as a volunteer commentedComment #54
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedNot following this context entirely but attached interdiff for clarity's sake.
Comment #55
LewisNyman CreditAttribution: LewisNyman at Wunder commentedLooks 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'
It looks like we are removing this function when we shouldn't. This was probably added in another issue.
Comment #56
njbarrett CreditAttribution: njbarrett as a volunteer commented@Lewis hopefully I understood you correctly - here's an updated patch and interdiff
Comment #57
LewisNyman CreditAttribution: LewisNyman at Wunder commented@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.Comment #58
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedComment #59
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedGetting back on this during Frontend United.
Comment #60
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedSo 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!
Comment #61
LewisNyman CreditAttribution: LewisNyman at Wunder commented@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,Comment #62
dawehnerMh, why do we need new selectors even we already have some?
Its a little bit odd to remove CSS without an obvious replacement ... ?
Comment #64
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedIs #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)
Comment #68
LewisNyman CreditAttribution: LewisNyman at Wunder commentedOk 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 :)
Comment #69
LewisNyman CreditAttribution: LewisNyman at Wunder commentedThe
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.See this comment in #10:
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 :)
Comment #70
dawehnerI think its wrong to blow up our markup for the non-common usecase, but well, this is what banana was about.
Comment #71
LewisNyman CreditAttribution: LewisNyman at Wunder commented@dawehner Don't worry we will trim your markup right down when we get the chance ;-)
Comment #72
alexpottBeing 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.
Comment #75
tim.plunkettThis change was incorrect, and visually looks silly. Why was this done?
Comment #76
joelpittetFrom 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?