Line height reduction #856

Heydon · 2023-01-03T13:29:56Z

Reduction of line-height values across brands.

Heydon · 2023-01-03T13:36:16Z

I've tidied up and normalized stuff as much as I think I can get away with. The only thing I've left that seems very wrong is the following, which represents a massive line-height but there may be a reason for that. There's no demo available for the package to check.

.c-header__journal-title {
	@include media-query('md') {
		font-size: 1.375rem;
	}

	@include media-query('lg') {
		font-size: 1.6875rem;
	}

	font-family: $context--font-family-serif;
	line-height: 2.875rem;
	font-size: .9375rem;
	color: color('black');
}

sturobson · 2023-01-05T16:14:14Z

although Sass compiles this to put the media queries after the initial declaration - I find it easier to understand if it's also written this way (see below).

So when authoring Sass it's: 'here's the general code and then if the browser is this big, do this' rather than 'if the browser is this big, do this, then do all this other stuff'

.c-header__journal-title {
	font-family: $context--font-family-serif;
	line-height: 2.875rem;
	font-size: .9375rem;
	color: color('black');

	@include media-query('md') {
		font-size: 1.375rem;
	}

	@include media-query('lg') {
		font-size: 1.6875rem;
	}
}

I've had a look in the front-end playbook and I couldn't spot guidance on this.

sturobson · 2023-01-05T16:16:15Z

I won't mention my favouring CSS written in alphabetical order - I'll leave that to another day.

alexkilgour · 2023-01-09T16:58:57Z

There is a lot going on here. Seems to include the publishing of a new component global-product?
Can the update to context be a separate PR than adding/updating components to make it easier to follow?

The design token updates are very noisy as well, it's very hard to tell what is going on. I'm not sure what is related to the line-height change and what isn't

alexkilgour · 2023-01-09T16:59:36Z

context/brand-context/HISTORY.md

@@ -1,5 +1,10 @@
 # History

+## 31.1.0 (2023-01-09)
+    * BREAKING:


If this is breaking, should it not be version 32?

@alexkilgour True, I forgot to sync that when I made it breaking.

alexkilgour · 2023-01-09T17:01:27Z

context/brand-context/default/scss/00-tokens/_icon.variables.scss

A lot of file seem to have this !default removed. Is this related at all to the line-height change or is this from something else and the tokens were not updated?

it's related, when Sass meets two !defaults for the same variable - it takes the first one, rather than the second. This meant that the overriding variable for the nature theme wasn't coming through.

Me adding !default was a pre-cursor for the single release but I got it wrong here.

Sadly I had added !default to pretty much every literal or alias token so we decided to remove them all, and 'circle back' to their usage later.

alexkilgour · 2023-01-09T17:02:28Z

context/brand-context/default/scss/00-tokens/_gap.variables.scss

@@ -1,6 +1,6 @@

 // Do not edit directly
-// Generated on Fri, 21 Oct 2022 11:11:43 GMT
+// Generated on Mon, 09 Jan 2023 13:28:25 GMT


There aren't actually any changes to this file, I think we need a way to not generate this line when the file doesn't change as it's extremely noisy

In fact is there any need for this "Generated on" message? Github shows the last changed date

I think we need a way to not generate this

Agreed, I have a task to look at seeing how we can 'test' what has changed and get Style Dictionary to act accordingly.

In fact is there any need for this "Generated on" message?

I guess not - as the 'do not edit directly' message is there.

Github shows the last changed date

We're assuming folks would check this‽ Although tools like VSCode are quite adept at showing this information (even line-by-line).

I think what we need is these files to be auto-generated as part of the package management, then there is no need fo anyone to check anything

Yes, I'd love some form of automation for this so it's not "yet another npm script" that someone needs to run. It not being on npmjs at the moment is probably scuppering possibilities.

🤞🏼 the single release 🤞🏼

jon-stevens · 2023-01-10T10:08:03Z

although Sass compiles this to put the media queries after the initial declaration - I find it easier to understand if it's also written that way.

So when authoring Sass it's: 'here's the general code and then if the browser is this big, do this' rather than 'if the browser is this big, do this, then do all this other stuff'
.c-header__journal-title {
	font-family: $context--font-family-serif;
	line-height: 2.875rem;
	font-size: .9375rem;
	color: color('black');

	@include media-query('md') {
		font-size: 1.375rem;
	}

	@include media-query('lg') {
		font-size: 1.6875rem;
	}
}
I've had a look in the front-end playbook and I couldn't spot guidance on this.

I think it makes more sense as you suggested. I seem to remember sass lint 'recommending' putting the media queries first. But I've just tested it in one of my local projects and no complaint. I may have dreamt it or changed linting config since..

Heydon · 2023-01-10T10:09:08Z

There is a lot going on here. Seems to include the publishing of a new component global-product?
Can the update to context be a separate PR than adding/updating components to make it easier to follow?

@alexkilgour That, naturally, shouldn't be there at all. My editor seems to like dropping one's branch's changes into another's. I'll unstage all that.

Heydon · 2023-01-10T10:10:33Z

although Sass compiles this to put the media queries after the initial declaration - I find it easier to understand if it's also written that way.

@jon-stevens Would you say this is relevant to this PR? I haven't created/touched those media queries.

dsmr · 2023-01-10T11:19:20Z

Hi Heydon

Me and Ben C have had a go at looking at the changes on your branch with npm link in our Oscar codebase. I think it is unrelated to the changes in this PR but we wanted to let you know that there is an issue in Brand Context that is not letting our builds complete:

Message:
    ../../node_modules/@springernature/brand-context/default/scss/40-base/block-spacing.scss
Error: Undefined variable.
   ╷
13 │     margin-block-start: $tokens--typography-block-spacing-medium;
   │                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ╵
  ../../node_modules/@springernature/brand-context/default/scss/40-base/block-spacing.scss 13:22  @import
  ../../node_modules/@springernature/brand-context/nature/scss/enhanced.scss 17:9                 @import
  css/homepage/homepage-150.scss 1:9                                                              root stylesheet
Details:
    formatted: Error: Undefined variable.
   ╷
13 │     margin-block-start: $tokens--typography-block-spacing-medium;
   │                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ╵
  ../../node_modules/@springernature/brand-context/default/scss/40-base/block-spacing.scss 13:22  @import
  ../../node_modules/@springernature/brand-context/nature/scss/enhanced.scss 17:9                 @import
  css/homepage/homepage-150.scss 1:9                                                              root stylesheet
    line: 13
    column: 22
    file: /Users/drt9804/IdeaProjects/oscar/lib/frontend/node_modules/@springernature/brand-context/default/scss/40-base/block-spacing.scss
    status: 1
    messageFormatted: ../../node_modules/@springernature/brand-context/default/scss/40-base/block-spacing.scss
Error: Undefined variable.
   ╷
13 │     margin-block-start: $tokens--typography-block-spacing-medium;
   │                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ╵
  ../../node_modules/@springernature/brand-context/default/scss/40-base/block-spacing.scss 13:22  @import
  ../../node_modules/@springernature/brand-context/nature/scss/enhanced.scss 17:9                 @import
  css/homepage/homepage-150.scss 1:9                                                              root stylesheet
    messageOriginal: Undefined variable.
   ╷
13 │     margin-block-start: $tokens--typography-block-spacing-medium;
   │                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   ╵
  ../../node_modules/@springernature/brand-context/default/scss/40-base/block-spacing.scss 13:22  @import
  ../../node_modules/@springernature/brand-context/nature/scss/enhanced.scss 17:9                 @import
  css/homepage/homepage-150.scss 1:9                                                              root stylesheet
    relativePath: ../../node_modules/@springernature/brand-context/default/scss/40-base/block-spacing.scss
    domainEmitter: [object Object]
    domainThrown: false

It appears as though the block-spacing.scss code in default brand context references a variable that doesn't exist: $tokens--typography-block-spacing-medium. We have run all of the scripts in the design-tokens package that generate Sass variables but this variable still does not exist.

For now we can manually override the code in block-spacing.scss to allow our frontend build to complete, which will allow us to test your changes on our sites locally. But we thought we would let you know as I guess that issue will need to be addressed separately to this PR.

Thanks!

sturobson · 2023-01-10T11:30:58Z

@dsmr

It appears as though the block-spacing.scss code in default brand context references a variable that doesn't exist: $tokens--typography-block-spacing-medium

$tokens--typography-block-spacing-medium was changed to $t-typography-block-spacing-medium (all generated tokens were changed to a shorthand t over tokens.

Taking a look at the block-spacing.scss file for the default brand it seems we missed renaming a couple of instances.

A hotfix/patch is incoming which should sort this out.

Sorry for the bother

EDIT

I have created the relevant PR that moves towards fixing any issues where we have $tokens-- which should be $t-.

EDIT EDIT

@dsmr this was resolved in #861 and is now released in v31.0.1 of the brand-context 🙏🏼

sturobson · 2023-01-10T11:45:20Z

Would you say this is relevant to this PR? I haven't created/touched those media queries.

@Heydon as I made the suggestion and it doesn't effect the PR we can ignore it for the interim I reckon.

Heydon added 10 commits November 25, 2022 17:34

wip, templates and scss

ce8a897

Merge branch 'main' of github.com:springernature/frontend-toolkits

0f3e281

line height changes and translations

4dc9d88

300 font weight removed

5e448ae

reinstate light font weight just for h1

09fa2c2

versioning

b817b1a

conversion to relative/unitless line-height values

ad6cb4a

submission header line-height

d426c1e

sn modal line-height

058dd79

date for brand context history

04477a5

Heydon requested a review from sturobson January 3, 2023 13:34

Heydon added 2 commits January 4, 2023 10:18

NAture line-height adjustments

1879c7a

font-weight range extended for serif font

51dd5ec

Heydon added 5 commits January 5, 2023 16:19

HISTORY font-weight ref

3d4dc15

removal of default flags

6735b54

larger nature line-height

d4e6c1a

Merge branch 'main' of github.com:springernature/frontend-toolkits

8b80bc6

Merge branch 'main' into line-height

ea4fa24

Heydon added the DO NOT MERGE label Jan 9, 2023

alexkilgour reviewed Jan 9, 2023

View reviewed changes

major version bump

11f0eb1

sturobson mentioned this pull request Jan 10, 2023

Update all component packages to latest brand-context version #858

Open

5 tasks

sturobson mentioned this pull request Jan 10, 2023

fix: updates compiled design tokens Sass variable names #861

Merged

Heydon and others added 3 commits January 10, 2023 12:29

deleted erroneous global-product files

68bb851

Merge branch 'main' into line-height

9400543

Merge branch 'main' into line-height

bfc6f08

sturobson requested review from foxintherain and removed request for foxintherain January 11, 2023 09:48

sturobson mentioned this pull request Jan 11, 2023

Swap Merriweather Sans font weight from 300 to 400 #849

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Line height reduction #856

Line height reduction #856

Heydon commented Jan 3, 2023

Heydon commented Jan 3, 2023

sturobson commented Jan 5, 2023 •

edited

Loading

sturobson commented Jan 5, 2023

alexkilgour commented Jan 9, 2023

alexkilgour Jan 9, 2023

Heydon Jan 10, 2023

alexkilgour Jan 9, 2023

sturobson Jan 10, 2023

alexkilgour Jan 9, 2023 •

edited

Loading

alexkilgour Jan 9, 2023

sturobson Jan 10, 2023

sturobson Jan 10, 2023

alexkilgour Jan 10, 2023

sturobson Jan 10, 2023

jon-stevens commented Jan 10, 2023 •

edited

Loading

Heydon commented Jan 10, 2023 •

edited

Loading

Heydon commented Jan 10, 2023

dsmr commented Jan 10, 2023 •

edited by benjclark

Loading

sturobson commented Jan 10, 2023 •

edited

Loading

sturobson commented Jan 10, 2023

Line height reduction #856

Are you sure you want to change the base?

Line height reduction #856

Conversation

Heydon commented Jan 3, 2023

Heydon commented Jan 3, 2023

sturobson commented Jan 5, 2023 • edited Loading

sturobson commented Jan 5, 2023

alexkilgour commented Jan 9, 2023

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

alexkilgour Jan 9, 2023 • edited Loading

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

jon-stevens commented Jan 10, 2023 • edited Loading

Heydon commented Jan 10, 2023 • edited Loading

Heydon commented Jan 10, 2023

dsmr commented Jan 10, 2023 • edited by benjclark Loading

sturobson commented Jan 10, 2023 • edited Loading

sturobson commented Jan 10, 2023

sturobson commented Jan 5, 2023 •

edited

Loading

alexkilgour Jan 9, 2023 •

edited

Loading

jon-stevens commented Jan 10, 2023 •

edited

Loading

Heydon commented Jan 10, 2023 •

edited

Loading

dsmr commented Jan 10, 2023 •

edited by benjclark

Loading

sturobson commented Jan 10, 2023 •

edited

Loading