PostCSS layout helpers inconsistency [#3374537]

Problem/Motivation

In the Olivero theme, there is an inconsistency in the use of spacing variables or layout helpers in the PostCSS files. While variables.pcss.css defines spacing variables like --sp1, --sp2, --sp3, etc., based on the variable --sp.

there are instances in some pcss files where values are directly assigned the `calc(2 * var(--sp));` value instead of using the equivalent --sp2. This inconsistency occurs for different versions of the layout helpers across several PCSS files.

I am mentioning --sp2 as an example but the situation is with different version of the layout helpers.

There are also instances of similar formulas, without a variable. In the spirit of standardization, we should add those to the variables.pcss and change the few occurrences.

Steps to reproduce

1. Open the PostCSS files in the Olivero theme.
2. Check for occurrences of the full formula instead of the corresponding variable.

Example in file core/themes/olivero/css/components/header-search-narrow.css line 47:

height: calc(3 * var(--sp));

Proposed resolution

To maintain consistency and readability, all instances of `calc([number] * var(--sp));` should be replaced with the equivalent --sp[number]. The same applies to other versions of the layout helpers. This will ensure that the usage of spacing variables is consistent across all PCSS files in the Olivero theme.

Test pages

The changes should be reflected in all pcss files of the Olivero theme.

Before/After screenshots

Screenshots are not applicable for this issue as it's a code consistency issue.

Markup changes

The markup changes involve replacing instances of `calc(2 * var(--sp));` with --sp2 (and similar replacements for other versions of the layout helpers) in the relevant PCSS files.

Cross browser tests

Cross-browser testing is not applicable for this issue as it's a code consistency issue.

No related issues.

Comment	File	Size	Author
#11	interdiff-4_11.txt	2.96 KB	gauravvvv
#11	3374537-11.patch	4.95 KB	gauravvvv
#4	3374537-postcss-layout-helpers-4.patch	2.6 KB	indrapatil

Issue fork drupal-3374537

Show commands

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

Add & fetch this issue fork’s repository

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

Add & fetch this issue fork’s repository

2 hidden branches

3374537-postcss-layout-helpers

changes, plain diff MR !4398

Check out this branch for the first time

Check out existing branch, if you already have it locally

11.x

changes, plain diff MR !4525

Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

13 July 2023 at 23:48

camoa created an issue. See original summary.

Comment #2

14 July 2023 at 00:33

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

Comment #3

indrapatil commented 14 July 2023 at 09:34

Assigned:

Unassigned

» indrapatil

I am working on it.

Comment #4

indrapatil commented 14 July 2023 at 09:40

Status:

Active

» Needs review

Status	File	Size
new	3374537-postcss-layout-helpers-4.patch	2.6 KB

I fixed the issue please review the patch.

Comment #5

indrapatil commented 14 July 2023 at 09:43

Assigned:

indrapatil

» Unassigned

Comment #6

narendrar

He/Him

English

Jaipur, India

commented 14 July 2023 at 10:17

Version:	10.1.x-dev	» 11.x-dev
Status:	Needs review	» Needs work

This needs to be done for all the instances of [number] * var(--sp) in Olivero theme. It seems it is only done for --sp3 only.

Comment #7

indrapatil commented 14 July 2023 at 10:30

Assigned:

Unassigned

» indrapatil

Comment #8

camoa commented 14 July 2023 at 13:13

Assigned:

indrapatil

» camoa

Comment #9

TY4DVenom commented 14 July 2023 at 17:22

I have changed all layout helper variables to fit the predetermined ones on variable.pcss.

Comment #10

camoa commented 14 July 2023 at 17:43

Status:

Needs work

» Needs review

Should we create instances that do not exist as variables but follow the pattern of the layout helpers? for consistency.

There are calc(13 * (var(--sp)); adn cal(4.5 * var(--sp)); that are not defined as variables.

Or is this too much?

Comment #11

gauravvvv commented 17 July 2023 at 03:10

Status	File	Size
new	3374537-11.patch	4.95 KB
new	interdiff-4_11.txt	2.96 KB

I have updated all the instances, attached interdiff for same. please review

Comment #12

narendrar

He/Him

English

Jaipur, India

commented 17 July 2023 at 06:15

Status:	Needs review	» Needs work
Issue tags:		+Needs issue summary update

Re #11 There are still instances which are not updated. Eg 0.5 * var(--sp).

Also I think #10 can also be covered under this issue. Adding IS update tag for this change.

Comment #13

camoa commented 17 July 2023 at 08:41

Why are we using patches when there is a fork already? Just curious.

The issue fork has the changes you mentioned and I will add the extra ones as soon as my day starts.

Comment #14

17 July 2023 at 09:00

camoa opened merge request !4398

Comment #15

camoa commented 18 July 2023 at 23:42

Issue summary:

View changes

Comment #16

camoa commented 18 July 2023 at 23:48

Comment #17

camoa commented 19 July 2023 at 02:33

Version:	11.x-dev	» 10.1.x-dev
Issue tags:	-Needs issue summary update

Updated summary, and changed back to 10.1.x since drupalpod is not working with 11.x apparently.

Comment #18

Harish1688 commented 20 July 2023 at 07:28

As per last comment #17 changes in MR, reviewing it

Comment #19

Harish1688 commented 20 July 2023 at 09:35

Status:

Needs work

» Needs review

Tested the MR 4398 for inconsistency in the use of spacing variables or layout helpers in the PostCSS files issue, like replacing the instances of `calc(2 * var(--sp)); calc(3 * var(--sp)); calc(4 * var(--sp)); ` with var(--sp2), var(--sp3) var(--sp4); and created new helper like : var(--sp3-5)l var(--sp13); are used in the relevant PCSS files are done.

Here are the steps I followed to test the changes:

1. Installed Drupal 10.1.x-dev version and navigated to the 'core/themes/olivero/css' path.
2. Switched to the MR (#4398) and verified the changes in the PCSS files.

Note: I didn't take screenshots because this issue is about code consistency, not visual changes.
Looks good for RTBC+

Comment #20

narendrar

He/Him

English

Jaipur, India

commented 20 July 2023 at 09:59

Status:

Needs review

» Needs work

Setting NW for CI failure. Also MR should be against 11.x. Changes should done only in pcss file and then yarn run build:css should be run to get those changes in css files.

Comment #21

camoa commented 1 August 2023 at 16:53

Version:

10.1.x-dev

» 11.x-dev

Comment #22

1 August 2023 at 20:50

camoa opened merge request !4525

Comment #23

1 August 2023 at 20:51

camoa closed merge request !4525

Comment #24

camoa commented 1 August 2023 at 21:05

I have worked out the rebase of Drupal 11.x and compiled CSS via yarn run build:css.

Comment #25

camoa commented 1 August 2023 at 21:16

Status:

Needs work

» Needs review

Comment #26

narendrar

He/Him

English

Jaipur, India

commented 2 August 2023 at 08:18

Status:

Needs review

» Needs work

Still some reference left in node-teaser.pcss.css and social-bar.pcss.css

Comment #27

23 August 2023 at 06:42

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

Comment #28

utkarsh_33 commented 23 August 2023 at 07:30

Status:

Needs work

» Needs review

Comment #29

narendrar

He/Him

English

Jaipur, India

commented 23 August 2023 at 08:05

Status:

Needs review

» Needs work

There seems one more --header-height-wide-when-fixed: calc(6 * var(--sp)); in variables.css and variables.pcss.css

Comment #30

utkarsh_33 commented 24 August 2023 at 08:01

Status:

Needs work

» Needs review

Comment #31

narendrar

He/Him

English

Jaipur, India

commented 24 August 2023 at 10:21

Status:

Needs review

» Reviewed & tested by the community

Changes looks good to me. Marking as RTBC.

Comment #32

longwave

he/him

English

commented 10 November 2023 at 23:10

Status:

Reviewed & tested by the community

» Fixed

I searched but could not find any other similar cases that could be replaced. I did wonder if there was a stylelint rule that could help us here but I am not sure you would necessarily want to replace all instances in all cases.

Minor spacing issue fixed on commit:

--- a/core/themes/olivero/css/layout/social-bar.pcss.css
+++ b/core/themes/olivero/css/layout/social-bar.pcss.css
@@ -23,7 +23,7 @@
   @media (--nav) {
     position: relative;
     width: var(--content-left);
-    padding-block:var(--sp5);
+    padding-block: var(--sp5);
     padding-inline-start: 0;
     padding-inline-end: 0;

Committed and pushed 418e5a48ac to 11.x and 3913db9d4d to 10.2.x and 2945fff811 to 10.1.x. Thanks!

Comment #33

10 November 2023 at 23:10

longwave committed 2945fff8 on 10.1.x

Issue #3374537 by camoa, Utkarsh_33, TY4DVenom, Gauravvvv, Indrapatil,...

Comment #34

10 November 2023 at 23:10

longwave committed 3913db9d on 10.2.x

Issue #3374537 by camoa, Utkarsh_33, TY4DVenom, Gauravvvv, Indrapatil,...

Comment #35

10 November 2023 at 23:10

longwave committed 418e5a48 on 11.x

Issue #3374537 by camoa, Utkarsh_33, TY4DVenom, Gauravvvv, Indrapatil,...

Comment #36

longwave

he/him

English

commented 10 November 2023 at 23:15

Version:	11.x-dev	» 10.1.x-dev
Assigned:	camoa	» Unassigned

Comment #37

24 November 2023 at 23:19

Status:

Fixed

» Closed (fixed)

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

Comment #38

quietone commented 4 October 2026 at 11:56

Issue tags:

-olivero

PostCSS layout helpers inconsistency

Problem/Motivation

Steps to reproduce

Proposed resolution

Test pages

Before/After screenshots

Markup changes

Cross browser tests

Related Issues

Issue fork drupal-3374537

Comments