Follow-up to #1342054: [META] Clean up templates and CSS

Problem/Motivation

  1. Bartik's template files need to be assessed and cleaned up of redundant markup, bad formatting and ID's.
  2. Bartik's CSS files need to follow Drupal's CSS Coding Standards.

Proposed resolution

For this issue we take "views.css" within Bartik in css/components/views.css plus any template file associated with the CSS and clean them up.

Formatting tasks to do

  1. Remove blank lines where necessary - guidelines are here.
  2. The CSS file needs to have the correct Comment formats - see guidelines here and also reference other fixed Bartik CSS files for wording guidelines.

CSS cleanup tasks to do

  1. Remove any CSS that repeats styles from elsewhere in Bartik and therefore are not needed in this file.
  2. Check all of the selector rules are correct and are currently in use on the frontend of Bartik.

Remaining tasks

  • Write a patch containing as much as the above as possible.
  • Post a patch with screenshots.
  • Visual review of a patch - check the elements being styles visually, with and without patch applied. Take screenshots.
  • Code review of a patch - check the code follows coding standards, suggest improvements if needed in a comment.
  • Produce a new patch with improvements if needed.

User interface changes

None

API changes

None

Data model changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is refactoring CSS and templates in Bartik
Issue priority Not critical because Bartik functions fine we are just doing cleanup tasks
Unfrozen changes Unfrozen because it only changes CSS and markup
Prioritized changes The main goal of this issue is usability of the Bartik's codebase
Disruption non-Disruptive as it is just changing markup and CSS
CommentFileSizeAuthor
#8 Screenshot 2015-09-01 18.06.56.jpg30.35 KBlewisnyman
#8 Screenshot 2015-09-01 18.06.24.jpg50.28 KBlewisnyman
#7 clean_up_the_views-2542600-7.patch3.2 KBemma.maria
#7 interdiff-6-7.txt355 bytesemma.maria
#6 clean_up_the_views-2542600-6.patch3.2 KBemma.maria
#6 interdiff-3-6.txt777 bytesemma.maria
#4 views-after.png16.37 KBemma.maria
#4 views-before.png15.61 KBemma.maria
#4 bartik-views-cleanup-visual-testing.pdf946.28 KBemma.maria
#3 clean_up_views-2542600-3.patch3.39 KBemma.maria
#3 interdiff-2-3.txt3.29 KBemma.maria
#2 clean_up_views-2542600-2.patch369 bytesmanjit.singh

Comments

manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented
Assigned: Unassigned » manjit.singh
manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented
Assigned: manjit.singh » Unassigned
Status: Active » Needs review
StatusFileSize
new clean_up_views-2542600-2.patch369 bytes
emma.maria’s picture

emma.maria
she/her
commented
Issue tags: -Novice
StatusFileSize
new interdiff-2-3.txt3.29 KB
new clean_up_views-2542600-3.patch3.39 KB

I noticed that there was a lot of redundant CSS code, selectors that could be improved, a lot of code that was being overwritten by the Views module that I made kick in / got rid of as it was not needed, and declarations of styles to improve.

As the list of improvements to make was going to be a huge list I knocked up a patch myself.

Patch and interdiff attached.

emma.maria’s picture

emma.maria
she/her
commented
Issue summary: View changes
StatusFileSize
new bartik-views-cleanup-visual-testing.pdf946.28 KB
new views-before.png15.61 KB
new views-after.png16.37 KB

Because the changes are big I have attached a visual regression report to show what's going on in the frontend.

There is only one difference which is an improvement.

Before

After

A code review is much needed please :)

lewisnyman’s picture

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

Great, I can confirm that there are no visual regressions. Just a few questions:

.views-display-column summary {
  text-shadow: none;
}

Why are we disabling this styling for a summary in this particular occasion? It feels inconsistent for the sake of it.

[dir="rtl"] .views-displays .tabs .action-list {
  padding-left: inherit;

This doesn't work as expected, as the styling is directly overriding from .region-content ul, .region-content ol. Usually in these situations we have to set the padding again but I would just remove all padding so you don't have to set it again for RTL.

/* Tabs action list styles */
.views-displays .tabs .action-list {
  padding: 0;
}
emma.maria’s picture

emma.maria
she/her
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-3-6.txt777 bytes
new clean_up_the_views-2542600-6.patch3.2 KB

I agree with the first point. Bartik declares the summary styling so we shouldn't override it just for Views.
Also good spot on the padding of .views-displays .tabs .action-list. I wondered what was going on there, padding: 0 creates no visual regressions.

I have made the changes in this patch.

PS. should we capitalise 'Views' in the file comment?

emma.maria’s picture

emma.maria
she/her
commented
StatusFileSize
new interdiff-6-7.txt355 bytes
new clean_up_the_views-2542600-7.patch3.2 KB

I added the capital letter to "Views" in the file comment. All other feedback has been addressed. Can someone please final check this for me?

lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screenshot 2015-09-01 18.06.24.jpg50.28 KB
new Screenshot 2015-09-01 18.06.56.jpg30.35 KB

Great, thanks for making these changes. I can confirm there are no regressions.


Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: clean_up_the_views-2542600-7.patch, failed testing.

emma.maria queued 7: clean_up_the_views-2542600-7.patch for re-testing.

rudraram’s picture

rudraram commented
Status: Needs work » Needs review
rudraram’s picture

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

Re-test passed so RTBC.

alexpott’s picture

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

Committed 2c62403 and pushed to 8.0.x. Thanks!

  • alexpott committed 2c62403 on 8.0.x 
    Issue #2542600 by emma.maria, Manjit.Singh, LewisNyman: Clean up the "...

Status: Fixed » Closed (fixed)

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