Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mismatch between scope: true and scope: false #28

Open
symroe opened this issue Jul 7, 2021 · 15 comments
Open

Mismatch between scope: true and scope: false #28

symroe opened this issue Jul 7, 2021 · 15 comments

Comments

@symroe
Copy link
Member

symroe commented Jul 7, 2021

There is a difference between the base units (for fonts and widths) when scope is true vs when it's false. I've not quite worked this out yet.

With no changes to the code at all, apart from flipping $scope: false; (and with class=ds-scope on the body) we get the following:

With scope

Screenshot 2021-07-07 at 08-05-57 About Democracy Club

Without scope

Screenshot 2021-07-07 at 08-06-25 About Democracy Club

The browser width is the same in both - and the font size is much bigger without scope.

I think this is due to using rems and the browser font size being bigger than expected, but I'm still investigating.

Tested on Firefox developer edition, Ubuntu.

@Heydon
Copy link
Collaborator

Heydon commented Jul 27, 2021

@symroe Sorry to be so late on this, I missed Virginia's email because i thought it related to the previous (solved) issue. We have used most of the support budget, just so you know, but I'm still able to investigate this. Is there a live view of this? What element is the text "Our vision is..." using?

@Heydon
Copy link
Collaborator

Heydon commented Jul 27, 2021

Looking at the system index file, it appears to be applying the same rule in either case.

@if $scope {
  .ds-scope {
    font-size: clamp(#{$s1}, 2vw, #{$s2});
    @include fontMain;
    @include type;
    @include forms;
    @include stack;
    @include optional-styles;
    @include dark;
    @include page;
    @include utilities;
  }

  .ds-scope *,
  .ds-scope *::before,
  .ds-scope *::after {
    box-sizing: border-box;
  }
} @else {
  :root {
    font-size: clamp(#{$s1}, 2vw, #{$s2});
    @include fontMain;
  }

  :root *,
  :root *::before,
  :root *::after {
    box-sizing: border-box;
  }

  body {
    margin: 0;
  }

  @include type;
  @include forms;
  @include stack;
  @include optional-styles;
  @include dark;
  @include page;
  @include utilities;
}

However, there may be issues regarding specificity (without the .ds-scope class, the original font-size might be applied to :root). Or there may be something going wrong because class="ds-scope" elements are being nested, which you shouldn’t do (I don’t think I was clear about this, so not your fault if so).

It might be worth testing what happens if you give a simple value for the :root font size like 100%:

@if $scope {
  .ds-scope {
    font-size: 100%;
    @include fontMain;
    @include type;
    @include forms;
    @include stack;
    @include optional-styles;
    @include dark;
    @include page;
    @include utilities;
  }

  .ds-scope *,
  .ds-scope *::before,
  .ds-scope *::after {
    box-sizing: border-box;
  }
} @else {
  :root {
    font-size: 100%;
    @include fontMain;
  }

  :root *,
  :root *::before,
  :root *::after {
    box-sizing: border-box;
  }

  body {
    margin: 0;
  }

  @include type;
  @include forms;
  @include stack;
  @include optional-styles;
  @include dark;
  @include page;
  @include utilities;
}

@symroe
Copy link
Member Author

symroe commented Jul 27, 2021

It might be worth testing what happens if you give a simple value for the :root font size like 100%

So, on the DC website there are no instances of nested ds-scope (but thanks for flagging - I'll check the other sites). Setting font-size: 100% on the root element makes the font sizes the same regardless of scope, but the .ds-page main width changes (because it's based on 70ch and the clamp alters that).

@Heydon
Copy link
Collaborator

Heydon commented Jul 27, 2021

Couple of things I noticed:

  1. (not related but important) as I've explained previously, heading elements should not be used just to make text bigger. The <h5> (pictured below) should be a <p> with class="ds-h5" (explained here: https://democracyclub.github.io/design-system/basics/type/).

h5 text

  1. The following rule appears to be added outside of the design system?
blockquote,
p {
 font-size:clamp(1rem, 1vw, 1.25rem)
}

It is causing paragraph text not to grow alonside list text, which looks weird:

With rule ❌

text different sizes

Without rule ☑️

text the same

I'm hoping this might help fix the issue? Difficult to test since removing ds-scope removes all the styles on the live site, with ds-scope being active and all.

Bonus bug fix:

The address is out of whack at the bottom.

address to left of logo

Quick fix is to add class="ds-width-full" to each of the paragraphs in the address. I added these utilities in a recent update with Virginia, so I'm glad they are paying for themselves!

@symroe
Copy link
Member Author

symroe commented Jul 27, 2021

Thanks Heydon. I've removed the styles for P, reverted the change you suggested above (e.g, I'm not setting font-size: 100%, just keeping system index defaults), and changed that H5.

Here's a gif of me flipping scope on and off (I'm using h1:after {content: " scope on"} to add the text)
output

So it looks like the base font is doing....something, and that in turn is messing with all the other relative sizes.

Happy to do a screen share if it's easier for you to debug

@Heydon
Copy link
Collaborator

Heydon commented Jul 27, 2021

Ah, I think the issue is around applying the font-size on :root/html as opposed to body.

The text is larger if added to :root i.e. not scoped.

To make the output consistent, the quickest fix is to add class="ds-scope" to <html> and not <body>. Otherwise, you could change the system CSS to use body like this:

...
} @else {
 body {
    font-size: clamp(#{$s1}, 2vw, #{$s2});
    @include fontMain;
  }

  body *,
  body *::before,
  body *::after {
    box-sizing: border-box;
  }
...

But this might have knock-on effects.

@Heydon
Copy link
Collaborator

Heydon commented Jul 27, 2021

Did you want classes like ds-h5 to have the same font styles as the <h5> counterpart? I could do a quick PR for that.

@symroe
Copy link
Member Author

symroe commented Jul 27, 2021

Ah, applying the styles to body rather than :root in the else worked, in that scope on/off is identical now.

Keeping :root rather than body, but moving the ds-scope class to the html tag works as well, in that they are both the same, but in this case everything is much bigger than it should be (as per the layout demo).

So, scope on HTML is the bigger text version of the above gif, using body in the CSS results in the smaller (and I think the better) one.

Are there any problems with using body rather than :root, when scope is off?

@symroe
Copy link
Member Author

symroe commented Jul 27, 2021

Did you want classes like ds-h5 to have the same font styles as the <h5> counterpart? I could do a quick PR for that.

I don't think so at the moment thanks — I think this is a one off where the first para is larger than normal, but we don't want to encourage using styles over tags in the case where things are actually headings.

@Heydon
Copy link
Collaborator

Heydon commented Jul 27, 2021

Are there any problems with using body rather than :root, when scope is off?

I think you're fine, especially since you now have control over the situation. The discrepancy is unexpected though—something to do with relative sizing and nesting I expect.

Apologies again for the lateness of getting back on this.

@symroe
Copy link
Member Author

symroe commented Jul 27, 2021

Not a problem, thanks for your help!

@symroe
Copy link
Member Author

symroe commented Jul 27, 2021

Sorry, one last thing: do you reckon it's reasonable to change this in the system index example?

@Heydon
Copy link
Collaborator

Heydon commented Jul 27, 2021

Do you mean the system.scss file?

@symroe
Copy link
Member Author

symroe commented Jul 27, 2021

No, I mean this file, and replacing all instances of :root with body

@Heydon
Copy link
Collaborator

Heydon commented Jul 27, 2021

Oh! Yes, good thinking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants