suggested commit message

git commit -m 'Issue #2542598 by xq1003, mori, emma.maria, lauriii: Clean up the "user" 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 "user.css" within Bartik in css/components/sidebar.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.

CSS architecture tasks to do

  1. Replace the combined elements and class rules. For eg. replace "div.password-suggestions" with just ".password-suggestions".

CSS file structure tasks

  1. Move any selectors that do not start with "user" to another appropriate CSS file that starts with the name of the first selector. For eg. "password-suggestions" styles should be moved to a password-suggestions.css file

CSS cleanup tasks to do

  1. Remove any CSS that repeats styles from elsewhere in Bartik and therefore 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
#20 clean_up_the_user-2542598-20.patch1.34 KBpjbaert
#13 interdiff-2542598-5-10.txt731 bytesxlin
#11 clean_up_the_user-2542598-10.patch1.34 KBxlin
#7 interdiff-2542598-1-5.txt1.35 KBdrupa11y
#5 clean_up_the_user-2542598-5.patch1.33 KBdrupa11y
#2 2542598-cleaning-up-css.diff2.3 KBxlin
#1 after.png25.85 KBxlin
#1 before.png25.67 KBxlin
#1 2542598-cleaning-up-css.patch2.3 KBxlin

Comments

xlin’s picture

xlin commented
StatusFileSize
new 2542598-cleaning-up-css.patch2.3 KB
new before.png25.67 KB
new after.png25.85 KB

Patch and screenshot attached. Should the selector .profile .field-name-field-user-picture be .profile .field-name-user-picture ?

xlin’s picture

xlin commented
StatusFileSize
new 2542598-cleaning-up-css.diff2.3 KB

Resubmit diff file.

xlin’s picture

xlin commented
Status: Active » Needs review
emma.maria’s picture

emma.maria
she/her
commented
Status: Needs review » Needs work

Thanks @xq1003 for the work. I noticed some improvements needed in the patch in #1.

  1. +++ b/core/themes/bartik/css/components/password-suggestions.css
@@ -0,0 +1,8 @@
+/**
+ * @file
+ * Password Suggestions.
+ */
+
+.password-suggestions {
+    border: 0;
+}
\ No newline at end of file

    A newline at the end of every CSS is needed. Please add here.

    2.  

  2. /**
 * @file
 * Password Suggestions.
 */

    Can this be rephrased to "Styles for password suggestions in Bartik".

    3.  

  3. +++ b/core/themes/bartik/css/components/user.css
@@ -1,8 +1,8 @@
 .profile .field-name-field-user-picture {
   float: none;
 }

    I checked the CSS in Core and the remaining code in user.css is no longer needed, so can we remove it, plus remove any references to the file in Bartik please.

Also @xq1003 in future can you please try to make interdiffs in a .txt format so the testbot does not test them. See here for instructions which includes naming conventions. Thanks.

drupa11y’s picture

drupa11y commented
StatusFileSize
new clean_up_the_user-2542598-5.patch1.33 KB

Made the above improvements.

drupa11y’s picture

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

drupa11y commented
StatusFileSize
new interdiff-2542598-1-5.txt1.35 KB
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Needs work 
+++ /dev/null
@@ -1,8 +0,0 @@
-  float: none;
-}

Double checked that this can be removed 1. class on that field is now .field-name-user-picture 2. It doesn't float.

Last thing to get fixed AFAIK:

+++ b/core/themes/bartik/css/components/password-suggestions.css
@@ -0,0 +1,8 @@
+ * Styles for password suggestions in Bartik

We are still missing period in the end of the sentence. Otherwise this is good to go!

xlin’s picture

xlin commented

@emma.maria, thanks for the detail clarification and advice. Will make sure to change .diff to .txt in the future :)

@lauriii, will work on the patch for missing period now.

emma.maria’s picture

emma.maria
she/her
commented

@xq1003 you are welcome. I look forward to reviewing your patch :)

xlin’s picture

xlin commented
Status: Needs work » Needs review
StatusFileSize
new clean_up_the_user-2542598-10.patch1.34 KB

Missing period added.

manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented

@xq1003 can you please upload a interdiff file so that reviewer can easily review the changes made in #11. Please refer https://www.drupal.org/documentation/git/interdiff

xlin’s picture

xlin commented
StatusFileSize
new interdiff-2542598-5-10.txt731 bytes

@Manjit.Singh, thank you for pointing it out. Interdiff file is attached.

manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented 
+++ b/clean_up_the_user-2542598-10.patch
@@ -13,13 +13,13 @@ index 009a67f..df74289 100644
 diff --git a/core/themes/bartik/css/components/password-suggestions.css b/core/themes/bartik/css/components/password-suggestions.css
 new file mode 100644
-index 0000000..77078fd
+index 0000000..cf2c2ba

No able to figure out what changes are done here.

lauriii’s picture

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

#11 addresses my last comments on #8. Thanks for taking the time and fixing this!

emma.maria’s picture

emma.maria
she/her
commented

Thanks @xq1003! RTBC++

emma.maria’s picture

emma.maria
she/her
commented
Issue summary: View changes

suggested commit message

git commit -m 'Issue #2542598 by xq1003, mori, emma.maria, lauriii: Clean up the "user" component in Bartik'

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: clean_up_the_user-2542598-10.patch, failed testing.

emma.maria’s picture

emma.maria
she/her
commented
Issue tags: +Needs reroll
pjbaert’s picture

pjbaert
Primary language Dutch
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new clean_up_the_user-2542598-20.patch1.34 KB

Rerolled the latest patch

manjit.singh’s picture

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

Thanks @pjbaert for rerolling the patch, its got passed by testbot, So changing the status to RTBC as per feedback by Lauriii and Emma in #15 and #16.

alexpott’s picture

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

Committed 79d71c2 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 79d71c2 on 8.0.x 
    Issue #2542598 by xq1003, mori, pjbaert, emma.maria, lauriii: Clean up...

Status: Fixed » Closed (fixed)

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