Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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
- Bartik's template files need to be assessed and cleaned up of redundant markup, bad formatting and ID's.
- 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
- 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.
- 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
| 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 |
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | comment_cleanup-2542572-2.patch | 491 bytes | gatorjoe |
| |||
Comments
Comment #1
gatorjoe CreditAttribution: gatorjoe as a volunteer commentedComment #2
gatorjoe CreditAttribution: gatorjoe as a volunteer commentedAdded proper file comment at the top and formatted commenting within the file to match coding standards.
Comment #3
lauriiiI 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.
Comment #4
emma.mariaRTBC++
Thanks @gatorjoe
Comment #5
drupal a11y CreditAttribution: drupal a11y as a volunteer commentedWhat about the "table ul.links"-styles? They don´t work well in my opinion.
See this:
Comment #6
drupal a11y CreditAttribution: drupal a11y as a volunteer commentedThe patch looks ok. Did a "Visual review of a patch." & "Code review of a patch.".
Comment #7
emma.maria@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.
Comment #8
drupal a11y CreditAttribution: drupal a11y as a volunteer commentedHope this issues is good for #5: https://www.drupal.org/node/2543928
Comment #9
lauriiiComment #10
lauriiiSuggested commit message
Add credit to mori for his help in the sprints.
Comment #11
Manjit.Singhjust correct a alphabetical order of properties :)
Other changes in #2 look good to me.
Comment #12
Manjit.Singhtriggering test bot for #11
Comment #13
LewisNyman@Manjit.Singh The alphabetical order of css properties is not an agreed standard. See: https://www.drupal.org/node/1887862#declaration-order
Comment #14
lauriiiCould 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.
Comment #15
LewisNymanI 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.
Comment #16
LewisNymanReuploading the patch from #2 which is RTBC.
No credit please. I've added the suggested commit message to the summary.
Comment #17
LewisNymanComment #18
lauriiiPatch from comment #2 is RTBC.
Comment #19
emma.mariaI 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'Comment #20
alexpottCommitted cf71c68 and pushed to 8.0.x. Thanks!