Spacing refactor rebased on the 3.0 feature branch #3971

lyubomir-popov · 2021-09-06T13:24:08Z

Done

[List of work items including drive-bys]

Fixes https://github.com/canonical-web-and-design/vanilla-squad/issues/1104

QA

Review refactoring and simplicity of use of the proposed systtem of spacing variables
Review Percy for any regressions
Review updated documentation:
- docs/settings/spacing-settings

webteam-app · 2021-09-06T13:24:10Z

Demo starting at https://vanilla-framework-3971.demos.haus

lyubomir-popov · 2021-09-07T18:10:03Z

@bartaz @sowasred2012 I think this is ready for review. Haven't ticked all the checkboxes as I am not sure whether they apply to the 3.0 feature branch, if they do I'll add those changes too, just let me know please.

remove some morre variables that are not needed add some clarifications and remove unnecessary var test change to see if percy works on this branch convert some remaining vars fix-typo update spacing md file remove unneeded markup fix table explanations fix typo fix tyypo

sowasred2012

Looks good, and Percy passes - just one question about the code.

sowasred2012 · 2021-09-09T09:06:48Z

scss/_patterns_pagination.scss

+        margin-left: -#{$sph--small};
      }

      &:last-child {
-        margin-right: -#{$sph-inner--small};
+        margin-right: -#{$sph--small};


Is interpolation needed in these cases? There's an example of a negative variable in another file in this PR here that doesn't use interpolation

I don't know why GH won't allow me to add a suggested change here, but yes, this interpolation could be removed.

sowasred2012 · 2021-09-09T09:07:56Z

scss/_patterns_side-navigation.scss

+      margin-left: -#{$sph--small};
+      margin-right: $sph--small;


Same question re: interpolation

bartaz

Looks great, thanks! Couple minor suggestions in inline comments.

scss/_base_button.scss

scss/_patterns_pagination.scss

bartaz · 2021-09-14T07:24:43Z

scss/_patterns_pagination.scss

+        margin-left: -#{$sph--small};
      }

      &:last-child {
-        margin-right: -#{$sph-inner--small};
+        margin-right: -#{$sph--small};


I don't know why GH won't allow me to add a suggested change here, but yes, this interpolation could be removed.

scss/_patterns_search-box.scss

scss/_patterns_side-navigation.scss

bartaz · 2021-09-14T07:29:56Z

templates/docs/settings/spacing-settings.md

@@ -7,70 +7,60 @@ context:
 # Spacing

 <hr>
+Spacing in Vanilla is controlled via a set of variables. There are two kinds of variables - nudges, which keep text aligned to the baseline grid, and multiples of the spacing unit, which define vertical and horizontal spacing.


Any reason why you list "nudges" always before standard 'multiples' spacing?

I think it would be clearer to do the opposite. The default (and easier to understand) are the spacing unit based variables, only if something doesn't align to baseline it uses nudges.
So in case of this sentence:

Suggested change

Spacing in Vanilla is controlled via a set of variables. There are two kinds of variables - nudges, which keep text aligned to the baseline grid, and multiples of the spacing unit, which define vertical and horizontal spacing.

Spacing in Vanilla is controlled via a set of variables. There are two kinds of variables - multiples of the spacing unit, which define vertical and horizontal spacing. and nudges, which keep text aligned to the baseline grid.

Precisely because they are more likely to be overlooked - especially if left after a second "and". they are also the firsrt things thst need to be set right, otherwise nothing would align to the baseline grid, and the multiples would be pointless.

bartaz · 2021-09-14T07:30:20Z

templates/docs/settings/spacing-settings.md


-Vanilla uses a default spacing unit of `.5rem` (`8px`) as a basis to calculate spacing inside and between components, as well as the line-heights of the different type sizes.
+- text baselines are "nudged" until they fall precisely on the grid
+- any margin, padding or other positioning happens in multiples of the baseline grid spacing unit.


I'd move this up above the point with nudges.

The nudges is what makes vertical rhythm work, I don't want them to feel like an optional final step - that's why I always put it at the top.

bartaz · 2021-09-14T07:31:01Z

templates/docs/settings/spacing-settings.md

+- text baselines are "nudged" until they fall precisely on the grid
+- any margin, padding or other positioning happens in multiples of the baseline grid spacing unit.
+
+## Nudges


I'd move the nudge section down to the bottom after other spacing is defined.

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

bartaz

Great work, thanks!

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

This reverts commit 540865f.

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

lyubomir-popov added the Don't merge label Sep 6, 2021

lyubomir-popov changed the base branch from master to vanilla-3.0 September 7, 2021 08:16

lyubomir-popov added Breaking change 💣 Documentation 📝 Documentation changes or updates Feature 🎁 New feature or request Maintenance 🔨 Review: Code needed Review: QA needed and removed Don't merge labels Sep 7, 2021

lyubomir-popov force-pushed the spacing-3.0 branch from 0f70420 to f8c0aca Compare September 8, 2021 08:38

lyubomir-popov added 2 commits September 8, 2021 10:59

remove mentions of density multiplier

bad8a8d

change circleci config to point to feature branch

ba07823

cristinadresch assigned bartaz and sowasred2012 Sep 9, 2021

sowasred2012 approved these changes Sep 9, 2021

View reviewed changes

sowasred2012 added Review: QA +1 and removed Review: QA needed labels Sep 9, 2021

cristinadresch unassigned sowasred2012 Sep 10, 2021

bartaz requested changes Sep 14, 2021

View reviewed changes

lyubomir-popov and others added 6 commits September 14, 2021 09:16

Update scss/_base_button.scss

5d86962

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

Update scss/_patterns_search-box.scss

36fd330

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

Update scss/_patterns_side-navigation.scss

4bafa04

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

Update scss/_base_button.scss

1d9d44f

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

Update scss/_patterns_pagination.scss

ceea81f

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

Update scss/_patterns_search-box.scss

cf052d6

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

bartaz approved these changes Sep 15, 2021

View reviewed changes

bartaz removed the Review: Code needed label Sep 15, 2021

bartaz added the Review: Code +1 label Sep 15, 2021

bartaz merged commit 540865f into canonical:vanilla-3.0 Sep 15, 2021

bartaz added a commit to bartaz/vanilla-framework that referenced this pull request Sep 15, 2021

Spacing refactor (canonical#3971)

55b7312

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

petesfrench added a commit to petesfrench/vanilla-framework that referenced this pull request Sep 16, 2021

Revert "Spacing refactor (canonical#3971)"

2e9d8a9

This reverts commit 540865f.

sowasred2012 pushed a commit that referenced this pull request Sep 29, 2021

Spacing refactor (#3971)

1fb135c

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

sowasred2012 pushed a commit that referenced this pull request Oct 6, 2021

Spacing refactor (#3971)

1e26ffa

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

sowasred2012 pushed a commit that referenced this pull request Oct 11, 2021

Spacing refactor (#3971)

9be2ad1

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

bartaz added a commit that referenced this pull request Oct 13, 2021

Spacing refactor (#3971)

cc56a5a

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

sowasred2012 pushed a commit that referenced this pull request Oct 21, 2021

Spacing refactor (#3971)

957e575

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

bartaz added a commit to bartaz/vanilla-framework that referenced this pull request Oct 25, 2021

Spacing refactor (canonical#3971)

9dbda24

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

bartaz added a commit that referenced this pull request Nov 3, 2021

Spacing refactor (#3971)

a5fcb5d

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

bartaz added a commit that referenced this pull request Nov 3, 2021

Spacing refactor (#3971)

93e48f9

Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spacing refactor rebased on the 3.0 feature branch #3971

Spacing refactor rebased on the 3.0 feature branch #3971

lyubomir-popov commented Sep 6, 2021 •

edited by bartaz

webteam-app commented Sep 6, 2021

lyubomir-popov commented Sep 7, 2021

sowasred2012 left a comment

sowasred2012 Sep 9, 2021

bartaz Sep 14, 2021

sowasred2012 Sep 9, 2021

bartaz left a comment

bartaz Sep 14, 2021

bartaz Sep 14, 2021

lyubomir-popov Sep 14, 2021

bartaz Sep 14, 2021

lyubomir-popov Sep 14, 2021

bartaz Sep 14, 2021

bartaz left a comment

	Spacing in Vanilla is controlled via a set of variables. There are two kinds of variables - nudges, which keep text aligned to the baseline grid, and multiples of the spacing unit, which define vertical and horizontal spacing.
	Spacing in Vanilla is controlled via a set of variables. There are two kinds of variables - multiples of the spacing unit, which define vertical and horizontal spacing. and nudges, which keep text aligned to the baseline grid.

Spacing refactor rebased on the 3.0 feature branch #3971

Spacing refactor rebased on the 3.0 feature branch #3971

Conversation

lyubomir-popov commented Sep 6, 2021 • edited by bartaz

Done

QA

webteam-app commented Sep 6, 2021

lyubomir-popov commented Sep 7, 2021

sowasred2012 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartaz left a comment

Choose a reason for hiding this comment

lyubomir-popov commented Sep 6, 2021 •

edited by bartaz