Suggested commit message

git commit -m 'Issue #2542572 by gatorjoe, mori, lauriii, emma.maria: Clean up the "Table" component in Bartik'

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 "table.css" within Bartik in css/components/table.css plus any template file associated with the CSS and clean them up.

Formatting tasks to do

  1. The CSS file needs to have the correct File Comment at the top of the page - see guidelines here and also reference other fixed Bartik CSS files for wording guidelines.
  2. Comments within the rest of the file need to have the correct format - use guidelines from point above also.

Remaining tasks

  • Write a patch containing as much as the above as possible.
  • Post a patch with screenshots.
  • Visual review of a patch.
  • Code review of a patch.
  • 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
#11 interdiff-2542572-2-11.txt882 bytesmanjit.singh
#11 comment_cleanup-2542572-11.patch1.09 KBmanjit.singh
#5 issue2542572_tables-ul.png48.23 KBdrupa11y
#2 comment_cleanup-2542572-2.patch491 bytesgatorjoe

Comments

gatorjoe’s picture

gatorjoe commented
Assigned: Unassigned » gatorjoe
gatorjoe’s picture

gatorjoe commented
Status: Active » Needs review
StatusFileSize
new comment_cleanup-2542572-2.patch491 bytes

Added proper file comment at the top and formatted commenting within the file to match coding standards.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community

I ran the CSS lint on this and it only shows error for using too specific selectors but that's only because system is using too specific selectors so we need to override them in Bartik. I couldn't find any other issues against Drupal CSS guidelines in tables.css so this looks good to me.

emma.maria’s picture

emma.maria
she/her
commented

RTBC++

Thanks @gatorjoe

drupa11y’s picture

drupa11y commented
StatusFileSize
new issue2542572_tables-ul.png48.23 KB

What about the "table ul.links"-styles? They don´t work well in my opinion.

See this:
table.css - table ul.links

drupa11y’s picture

drupa11y commented

The patch looks ok. Did a "Visual review of a patch." & "Code review of a patch.".

emma.maria’s picture

emma.maria
she/her
commented

@mori thank you for finding table bugs in #5. These existed before the clean up so we can raise a follow up for these things :). Would you be up to creating that one? Here's a tutorial about filing good issues.

This issue is still RTBC.

drupa11y’s picture

drupa11y commented

Hope this issues is good for #5: https://www.drupal.org/node/2543928

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Assigned: gatorjoe » Unassigned
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue summary: View changes

Suggested commit message

Add credit to mori for his help in the sprints.

manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented
StatusFileSize
new comment_cleanup-2542572-11.patch1.09 KB
new interdiff-2542572-2-11.txt882 bytes

just correct a alphabetical order of properties :)

Other changes in #2 look good to me.

manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented
Status: Reviewed & tested by the community » Needs review

triggering test bot for #11

lewisnyman’s picture

lewisnyman
he/him
commented

@Manjit.Singh The alphabetical order of css properties is not an agreed standard. See: https://www.drupal.org/node/1887862#declaration-order

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Needs work

Could you please look the declaration order which is given here. We are not ordering CSS properties alphabetically but instead with groups. How properties are being ordered inside the group haven't been clearly defined.

lewisnyman’s picture

lewisnyman
he/him
commented

I would advise over re-ordering them for now. It's very hard to track CSS that has change if we also move it around. We can easily do the reordering in 8.1 without breaking anything.

lewisnyman’s picture

lewisnyman
he/him
commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Reuploading the patch from #2 which is RTBC.

No credit please. I've added the suggested commit message to the summary.

lewisnyman’s picture

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

lauriii
he/him
Primary language Finnish
Location Finland
commented

Patch from comment #2 is RTBC.

emma.maria’s picture

emma.maria
she/her
commented

I agree with the suggested commit message added by lewisnyman to the issue summary:

Suggested commit message

git commit -m 'Issue #2542572 by gatorjoe, mori, lauriii, emma.maria: Clean up the "Table" component in Bartik'

alexpott’s picture

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

Committed cf71c68 and pushed to 8.0.x. Thanks!

  • alexpott committed cf71c68 on 8.0.x 
    Issue #2542572 by gatorjoe, mori, lauriii, emma.maria: Clean up the "...

Status: Fixed » Closed (fixed)

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