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
#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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mherchel created an issue. See original summary.

mherchel’s picture

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

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

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
FileSize
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
CreditAttribution: mherchel 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
CreditAttribution: ckrina at Lullabot commented
Status: Needs review » Reviewed & tested by the community

LGTM!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
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 CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3312636-9.patch10.43 KB

Addressed #8, attached patch for same.

mherchel’s picture

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

Is there an interdiff to see what changed?

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association 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 CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative
FileSize
Screenshot 2023-03-20 at 3.19.44 PM.png102.23 KB
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.