The current details stylesheet doesn't make use of PostCSS nesting, custom properties, etc.

We can refactor this to make the code more portable, modular, and readable.

CommentFileSizeAuthor
#18 Refactor after1.png57.83 KBsagarmohite0031
#18 Refactor after.png46.27 KBsagarmohite0031
#12 Screenshot 2023-03-20 at 3.19.49 PM.png132.14 KBsmustgrave
#12 Screenshot 2023-03-20 at 3.19.44 PM.png102.23 KBsmustgrave
#9 3312636-9.patch10.43 KBgauravvvv
#8 after.png18.1 KBlauriii
#4 3312636-4-reroll.patch10.62 KBmherchel
#2 3312636.patch12.31 KBmherchel

Issue fork drupal-3312636

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

Comments

mherchel created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3312636.patch12.31 KB
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Active » Needs review
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3312636-4-reroll.patch10.62 KB

Re-roll within minor changes attached.

Minor changes

  • Removed unnecessary comment
  • Re-ordered logical properties
  • Added a period to another comment
  • Changed name of a custom property to be more consistent.

Status: Needs review » Needs work

The last submitted patch, 4: 3312636-4-reroll.patch, failed testing. View results

mherchel’s picture

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

Random test failures are no longer failing. This is ready for review.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Needs review » Reviewed & tested by the community

LGTM!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new after.png18.1 KB 
  1. +++ b/core/themes/olivero/css/components/details.css
@@ -23,50 +23,74 @@
-  --details-summary-transition: background-color 0.12s ease-in-out;
...
+  transition: background-color 0.2s;

    Why are we changing the transition? 

  2. +++ b/core/themes/olivero/css/components/details.pcss.css
@@ -5,110 +5,96 @@
-    margin-block-start: var(--sp1-5);
-    margin-block-end: var(--sp1-5);
-    margin-inline-start: var(--sp2);
-    margin-inline-end: var(--sp2);
+    margin: calc(2 * var(--details-spacing-unit)) calc(1.5 * var(--details-spacing-unit));

    This slightly changes the margin inside the details. I think this looks better with the value before the change because the elements inside the wrapper are aligned with the summary.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3312636-9.patch10.43 KB

Addressed #8, attached patch for same.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

Is there an interdiff to see what changed?

gauravvvv’s picture

gauravvvv commented

I tried applying patch #4, but I was getting an error. That's why didn't provided interdiff.

 git apply -v 3312636-4-reroll.patch
Checking patch core/themes/olivero/css/components/details.css...
error: while searching for:

/* Width of the entire grid maxes out. */

:root {
  --details-border-width: 1px;
  --details-summary-transition: background-color 0.12s ease-in-out;
}

.olivero-details {
  display: block;
  margin-block: var(--sp1);
  color: inherit;
  border: var(--details-border-width) solid var(--color--gray-95);
  border-radius: var(--border-radius);
  box-shadow: 0 1px 4px var(--color--gray-90);
}

/* Details summary styles */

.olivero-details__summary {
  position: relative;
  padding-block: var(--sp1);
  padding-inline-start: var(--sp2);
  padding-inline-end: var(--sp1);
  list-style: none;
  cursor: pointer;
  transition: var(--details-summary-transition);
  word-wrap: break-word;
  -webkit-hyphens: auto;
  hyphens: auto;
  color: inherit;
  background-color: var(--color--gray-100);
  font-size: var(--line-height-s);
  font-weight: 700;
  line-height: var(--sp1);
}

/* Arrow icon */

.olivero-details__summary:before {
  position: absolute;
  inset-block-start: 50%;
  inset-inline-start: var(--sp0-75);
  display: block;
  width: 0.625rem;
  height: 0.625rem;
  content: "";
  transform: translateY(-50%) rotate(45deg); /* LTR */
  border-top: solid 2px currentColor;
  border-right: solid 2px currentColor;

error: patch failed: core/themes/olivero/css/components/details.css:23
error: core/themes/olivero/css/components/details.css: patch does not apply
Checking patch core/themes/olivero/css/components/details.pcss.css...

I have replaced transition: background-color 0.2s; with 

--details-summary-transition: background-color 0.12s ease-in-out;
transition: var(--details-summary-transition);

Also, replaced margin: calc(2 * var(--details-spacing-unit)) calc(1.5 * var(--details-spacing-unit)); with margin: var(--sp1-5) var(--sp2); in the patch.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative
StatusFileSize
new Screenshot 2023-03-20 at 3.19.44 PM.png102.23 KB
new Screenshot 2023-03-20 at 3.19.49 PM.png132.14 KB

Tested out by enabling Olivero when creating content
When to node/add/page
The details section doesn't seem broken. Here's a few screenshots but let me know if there's another section to check.

1

2

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 3312636-9.patch, failed testing. View results

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

saurav-drupal-dev made their first commit to this issue’s fork.

saurav-drupal-dev opened merge request !10253

saurav-drupal-dev’s picture

saurav-drupal-dev commented
Status: Needs work » Needs review

Create MR from patch 312636-9.patch

sagarmohite0031’s picture

sagarmohite0031 commented
StatusFileSize
new Refactor after.png46.27 KB
new Refactor after1.png57.83 KB

Hello,
Tested and verified before applying MR.
Not able to reproduce issue looks good to me.
Attaching screenshots.

smustgrave’s picture

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

@sagarmohite0031 there is not a need for additional screenshots when the ticket already has them. #12 I added screenshots and there's been no change since. Thanks

Restoring previous status.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

small nit left, it's a bit longer but it's more consistent.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.