Problem/Motivation

This is part of the CSS modernization initiative. This is intended to be a straightforward second issue.

The first issue was regarding the entity-meta stylesheet.

Steps to reproduce

The stylesheet at https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/themes/claro/css/components/entity-meta.pcss.css needs to be refactored to make use of modern CSS and Drupal core's PostCSS tooling.

Proposed resolution

Use CSS Logical Properties where appropriate
Use CSS nesting where appropriate

User interface changes

None. There should be no visual differences.

CommentFileSizeAuthor
#36 Create_Article___Drupal.png449.33 KBmherchel
#15 3294003-version10.0.x.patch2.98 KBAditya4478
#13 3294003-10.0.x.patch2.93 KBAditya4478
#12 3294003-9.5.x.patch2.62 KBAditya4478
#10 3294003-9.5.x-10.patch2.52 KBMunavijayalakshmi
#7 3294003-9.5.x.patch2.63 KBAditya4478
#4 interdiff_2-4.txt403 bytesmrinalini9
#4 3294003-4.patch2.51 KBmrinalini9
#2 3294003.patch2.62 KBAditya4478

Issue fork drupal-3294003

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sasanikolic created an issue. See original summary.

Aditya4478’s picture

Aditya4478 CreditAttribution: Aditya4478 commented
Status: Active » Needs review
FileSize
3294003.patch2.62 KB
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work

Super minor but
+ background-color: #fff
Missing semi colon

mrinalini9’s picture

mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3294003-4.patch2.51 KB
interdiff_2-4.txt403 bytes

Updated patch #2 by addressing #3, please review it.

sasanikolic’s picture

sasanikolic CreditAttribution: sasanikolic at Google Summer of Code commented
Status: Needs review » Needs work 
+++ b/core/themes/claro/css/components/entity-meta.pcss.css
@@ -15,6 +15,21 @@
+  & .form-type--item .form-item__label {

I think this can be nested inside the .form-item definition above, no?

The semicolon shouldn't be an issue. That is caused by our compiler, perhaps we need to check why that happens at some point.

sasanikolic’s picture

sasanikolic CreditAttribution: sasanikolic at Google Summer of Code commented

Sorry, I meant that the after pseudo element can be nested inside the & .form-type--item .form-item__label definition.

Aditya4478’s picture

Aditya4478 CreditAttribution: Aditya4478 commented
Status: Needs work » Needs review
FileSize
3294003-9.5.x.patch2.63 KB
sasanikolic’s picture

sasanikolic CreditAttribution: sasanikolic at Google Summer of Code commented
Status: Needs review » Needs work

Changes look good to me if you make the patch apply correctly. :)

Munavijayalakshmi’s picture

Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound for Valuebound commented
Assigned: Unassigned » Munavijayalakshmi
Munavijayalakshmi’s picture

Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound for Valuebound commented
FileSize
3294003-9.5.x-10.patch2.52 KB

added patch for rectifying CI errors.

Munavijayalakshmi’s picture

Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound for Valuebound commented
Assigned: Munavijayalakshmi » Unassigned

@sasanikolic
can you please tell me how to rectify custom commands failed error.
I am new to drupal core contributions.
i am not understand what is mistake in core/themes/claro/css/components/entity-meta.pcss.css.

Aditya4478’s picture

Aditya4478 CreditAttribution: Aditya4478 commented
Status: Needs work » Needs review
FileSize
3294003-9.5.x.patch2.62 KB

No block for IE in this file. It's good to go for review.

Aditya4478’s picture

Aditya4478 CreditAttribution: Aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
FileSize
3294003-10.0.x.patch2.93 KB

Patch for Version 10.0.x

sasanikolic’s picture

sasanikolic CreditAttribution: sasanikolic at Google Summer of Code commented
Version: 10.0.x-dev » 9.5.x-dev
Status: Needs review » Needs work 
+++ b/core/themes/claro/css/components/entity-meta.pcss.css
@@ -15,6 +15,21 @@
+    margin-top: var(--size-entity-meta-spacing);

Let's use margin-block-start and margin-block-end here for the D10 patch.

Aditya4478’s picture

Aditya4478 CreditAttribution: Aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
Status: Needs work » Needs review
FileSize
3294003-version10.0.x.patch2.98 KB

Please consider #12 for D9 and for D10 consider this patch.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
CreditAttribution: ckrina at Lullabot commented
Parent issue: » #3324398: [META] Update Claro CSS with new coding standards
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
CreditAttribution: ckrina at Lullabot commented
Issue summary: View changes

bspeare made their first commit to this issue’s fork.

bspeare opened merge request !3212

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Status: Needs review » Needs work

Sorry the MR took me so long to review! It's looking awesome and is really close!

1. The MR needs to be changed to 10.1.x (right now it's 10.0.x). Only the creator of the MR can do so.
2. Once that's changed, bring it up to date, and recompile the CSS.
3. I left a number of minor comments in the MR that need to be addressed.
4. 🙌🙌🙌

Gauravvv made their first commit to this issue’s fork.

bspeare opened merge request !3284

bspeare closed merge request !3212

mherchel opened merge request !3288

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Status: Needs work » Needs review

I opened up the MR above when the current one wasn't in a good state. That being said the current one is currently green.

Pretty sure this should be set to Needs Review. I'll review it at some point :D

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work

Could the MR be updated for 10.1? See a lot of changes that don't have to do with claro

Thanks!

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup +Needs Review Queue Initiative

Nesting looks good and variable use.

lauriii closed merge request !3288

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

I think the code on merge request 3284 looks good. Posted one question on the MR about an open todo comment.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer and at Eclipse Foundation commented
Status: Reviewed & tested by the community » Needs work

Let's remove the todos, if it's important create an issue for it in the queue.

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Status: Needs review » Needs work

This is looking really close! We need to 1) make sure that there's an empty line between the code blocks and 2) There's one place where nesting can be used.

rpayanm made their first commit to this issue’s fork.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
CreditAttribution: rpayanm at Agileana commented
Status: Needs work » Needs review

I applied the suggestions.
Please review.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
Create_Article___Drupal.png449.33 KB

Looks perfect. Thank you!

Amber Himes Matz’s picture

Amber Himes Matz
she/her
Primary language English
Location Portland, OR USA
CreditAttribution: Amber Himes Matz at Drupalize.Me commented
Status: Reviewed & tested by the community » Needs work

Hiding the patch file because it looks like the workflow was switched to MR.

The MR is currently blocked according to GitLab and needs to be rebased.

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review

I have rebased the MR. It is ready to merge.

Amber Himes Matz’s picture

Amber Himes Matz
she/her
Primary language English
Location Portland, OR USA
CreditAttribution: Amber Himes Matz at Drupalize.Me commented

There appear to be a number of unresolved threads on the MR that will need to be resolved before this can be RTBC’d again. Thank you!

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work
Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review

Fixed build failure, Also all the feedbacks are addressed. please review

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Think this is good now.

  • lauriii committed 243be9a2 on 11.x 
    Issue #3294003 by bspeare, Aditya4478, Gauravvvv, mherchel, sasanikolic...

lauriii closed merge request !3284

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Version: 10.0.x-dev » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 243be9a and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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