Sass changes #50

osk · 2017-09-04T11:40:18Z

Changes to default Sass:

Add scaling css var and calc to responsive-font to deal with wide pages
Move hardcoded values to variables
Deprecate viewport mixin via @warn
Update $min-mobile breakpoint to 420 from 480
Update $page-width to 1290px
Rename (and deprecate old) grid mixins to grid-col etc
Add heading() subheading and copy() mixinstemplates

Variables: $grid-column-count and $grid-baseline Deprecate: make-row (now grid-row) make-col (now grid-col) make-offset-left (now grid-offset-left) make-offset-right (now grid-offset-right)

organdonor47

maybe tidy that container mixin? added a comment on that.

also, we could add that weird dimensions breakpoint fix that we talked about & is in this PR - https://github.com/ueno-llc/ueno-www/pull/342 -
(will need to be documented & maybe even commented out by default?)

organdonor47 · 2017-09-04T12:03:16Z

shared/styles/mixins.scss

@@ -83,7 +97,7 @@
  padding: 0 $container-gutter-mobile;
  max-width: $page-width + $container-gutter-mobile * 1;


this mobile stuff in container doesn't really do anything; $page-width is always going to be greater than mobile viewports anyway and I am not sure why there are differences between the base size ($page-width + $container-gutter-mobile * 1) and min-mobile anyway?

also, there may be instances where we need to override top & bottom padding / margins, so this would be over-written at the next mq:

&__container { @include container; padding-top: percentage(50px/$page-limit); // overwritten at tablet to 0; }

could left & right be explicitly declared as long-hand values rather than short-hand?

Like:

@mixin container() { margin-left: auto; margin-right: auto; padding-left: $container-gutter-mobile; padding-right: $container-gutter-mobile; @media (min-width: $min-tablet) { padding-left: $container-gutter-tablet; padding-right: $container-gutter-tablet; max-width: $page-width + $container-gutter-tablet * 2; } @media (min-width: $min-desktop) { padding-left: $container-gutter-desktop; padding-right: $container-gutter-desktop; max-width: $page-width + $container-gutter-desktop * 2; } }

It should definitely not use shorthand for padding.

geirlandro · 2017-09-04T13:20:28Z

shared/styles/grid.scss

+@mixin make-row($direction: ltr, $align: stretch, $justify: flex-start, $grid-gutter: $gutter, $wrap: wrap) {
+  @warn 'make-row mixin is deprecated, use grid-row';
+  @include grid-row($direction, $align, $justify, $grid-gutter, $wrap);
+}


You can do:

@mixin make-row($args...) { ... @include grid-row($args...); }

osk · 2017-09-04T13:36:34Z

Yeah, I had the scale thingie in mind. Has it been reviewed? Or should I just port it here and add to a page for easier review?

organdonor47 · 2017-09-04T13:38:22Z

yeah @osk maybe add in from that PR & we can test here instead -- not sure it needs to be implemented for ueno.co anyway (or at least there is no urgent need)

bjarnif · 2017-09-04T15:02:11Z

It would be nice to add these to html {} in base.scss
-webkit-font-smoothing: antialiased; // stylelint-disable-line
-moz-osx-font-smoothing: grayscale; // stylelint-disable-line

osk · 2017-09-05T13:41:00Z

I've updated according to comments.

@organdonor47 I'll add the scale experiment in a new branch after this one is merged.

First version with simple calculation with fallback

osk · 2017-09-05T17:31:01Z

On second thought, added a first version of scaling to responsive-font

j2em6h · 2017-09-05T17:26:31Z

shared/styles/grid.scss

  margin-left: convertify($offset);
 }

-@mixin make-offset-right($offset: percentage(1/12)) {
+@mixin grid-offset-right($offset: percentage(1/$grid-column-count)) {


Could the default $offset: just be 1 on lines 42 and 46? Because the percentage($number / $grid-column-count) is already getting applied via convertify($offset)?

Yeah, since that's the expected behaviour when you're using it

j2em6h · 2017-09-05T17:28:33Z

shared/styles/mixins.scss

+  @include reset-heading;
+  @include responsive-font($min, $max);
+
+  @warn 'Please implement for project';


What does implement for project mean?

These three mixins are included for those that want them. They have some settings that need to be changed for the project you're using them for, so this should trigger you to change them. But the wording is bad.

Please set values in this mixin for your project.?

Hmmm, are these really that different than any other piece of the starter kit? Anything could be customized for a project if it needs to be, right?

True, but these values are arbitrary, like the 58 for margin, other values are based on $gutter or have some logic behind them.

It doesn't have to be arbitrary, it could be based off something. Maybe even ($gutter * 2); or some other new variable.

@k80 @osk i think if we add a variable there it could be mistaken for an integral non-editable part of the mixin though. an arbitrary value might look a bit messy, but at least it indicates a value you can edit? maybe we could have warnings in there to update those values and just have some kind of obvious placeholder value?

123456789px would be obvious… and totally break the design.

j2em6h · 2017-09-05T17:30:26Z

shared/styles/mixins.scss

@@ -83,17 +97,69 @@
  padding: 0 $container-gutter-mobile;


Should this also be long hand?

Hmm, I'm not sure. Is there a case where we need to explicitly force it to be 0? @organdonor47 @mdmagnusson ?

@osk @k80 yeh it should i think. no reason why it needs to be 0 top & bottom

j2em6h · 2017-09-06T00:09:48Z

shared/styles/mixins.scss

+  font-family: $font-family;
+  font-weight: normal;
+  letter-spacing: 0;
+  line-height: (58/52);


Is this /52 supposed to be /$max? It is in the other 2.

grid-offset-* default value 1 instead of 1/$grid-column-count container mixin uses padding longhand heading, subheading and copy mixins don't warn, just have explicitly arbitrary values

…t-universally into feature/scss-changes

osk · 2017-09-11T10:19:26Z

@k80 @organdonor47
I removed the @warn from the mixins and set explicit $arbitrary-* variables instead it would always be changed, right? 😊

osk · 2017-09-13T10:11:36Z

I'm merging this later today if there are no objections

organdonor47 · 2017-09-13T11:20:56Z

i think there is probably a more sensible solution to this down the line, but this is as good as any other alternative I can think of!

osk added 9 commits August 31, 2017 12:54

Move grid constants into vars, rename and deprecate grid mixins

90ed7c3

Variables: $grid-column-count and $grid-baseline Deprecate: make-row (now grid-row) make-col (now grid-col) make-offset-left (now grid-offset-left) make-offset-right (now grid-offset-right)

Deprecate viewport mixin, name breakpoints

a2b3def

Move all hardcoded values into variables

2b0ba40

Update mobile breakpoint from 480 to 420

3315fa7

Update page-width to 1290, the ueno grid width

5b90c3f

Changes gathered from other projects

8af2b7a

Add warning to responsive-font if min-size >= size

d550d5f

Bug fixes

b37df54

Add heading() subheading and copy() mixins templates

944c86e

birkir temporarily deployed to starter-kit-universally-pr-50 September 4, 2017 11:40 Inactive

osk requested review from gakimball, mdmagnusson, j2em6h and organdonor47 September 4, 2017 11:40

organdonor47 reviewed Sep 4, 2017

View reviewed changes

geirlandro reviewed Sep 4, 2017

View reviewed changes

osk added 4 commits September 5, 2017 13:20

Use sass variable arguments syntax for deprecated mixins

0e84cd0

Add vendor prefixed font smoothing to base

4e6ffc0

Use longhand for media query padding in container mixin

ff4e3dd

Lint and decrease usage of stylelint-disable-line

92b46ae

osk temporarily deployed to starter-kit-universally-pr-50 September 5, 2017 13:34 Inactive

osk added 2 commits September 5, 2017 17:29

Add scaling css var and calc to responsive-font to deal with wide pages

c557a72

First version with simple calculation with fallback

Add Heading component to test and showcase responsive-font

1ac045a

osk temporarily deployed to starter-kit-universally-pr-50 September 5, 2017 17:30 Inactive

j2em6h reviewed Sep 5, 2017

View reviewed changes

j2em6h reviewed Sep 6, 2017

View reviewed changes

Update subheading mixin

6a6c16e

osk had a problem deploying to starter-kit-universally-pr-50 September 6, 2017 08:27 Failure

osk added 2 commits September 11, 2017 10:15

Update after review

dc9df79

grid-offset-* default value 1 instead of 1/$grid-column-count container mixin uses padding longhand heading, subheading and copy mixins don't warn, just have explicitly arbitrary values

Merge branch 'feature/scss-changes' of github.com:ueno-llc/starter-ki…

70aafaa

…t-universally into feature/scss-changes

osk deployed to starter-kit-universally-pr-50 September 11, 2017 10:17 View deployment

osk merged commit 655d039 into development Sep 14, 2017

osk deleted the feature/scss-changes branch September 14, 2017 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sass changes #50

Sass changes #50

osk commented Sep 4, 2017 •

edited

Loading

organdonor47 left a comment

organdonor47 Sep 4, 2017 •

edited

Loading

osk Sep 4, 2017

geirlandro Sep 4, 2017

osk commented Sep 4, 2017

organdonor47 commented Sep 4, 2017

bjarnif commented Sep 4, 2017

osk commented Sep 5, 2017

osk commented Sep 5, 2017

j2em6h Sep 5, 2017

osk Sep 5, 2017

j2em6h Sep 5, 2017

osk Sep 5, 2017

j2em6h Sep 6, 2017

osk Sep 6, 2017

j2em6h Sep 6, 2017

organdonor47 Sep 6, 2017 •

edited

Loading

j2em6h Sep 6, 2017

j2em6h Sep 5, 2017

osk Sep 5, 2017

organdonor47 Sep 6, 2017

j2em6h Sep 6, 2017

osk commented Sep 11, 2017

osk commented Sep 13, 2017

organdonor47 commented Sep 13, 2017

		@@ -83,7 +97,7 @@
		padding: 0 $container-gutter-mobile;
		max-width: $page-width + $container-gutter-mobile * 1;

Sass changes #50

Sass changes #50

Conversation

osk commented Sep 4, 2017 • edited Loading

organdonor47 left a comment

Choose a reason for hiding this comment

organdonor47 Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osk commented Sep 4, 2017

organdonor47 commented Sep 4, 2017

bjarnif commented Sep 4, 2017

osk commented Sep 5, 2017

osk commented Sep 5, 2017

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

organdonor47 Sep 6, 2017 • 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

osk commented Sep 11, 2017

osk commented Sep 13, 2017

organdonor47 commented Sep 13, 2017

osk commented Sep 4, 2017 •

edited

Loading

organdonor47 Sep 4, 2017 •

edited

Loading

organdonor47 Sep 6, 2017 •

edited

Loading