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.

Sign up for GitHub

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

Jump to bottom

Different way of setting up modular spacing #704

Merged
merged 25 commits into from
Nov 5, 2020
Merged

Different way of setting up modular spacing #704

merged 25 commits into from
Nov 5, 2020

Conversation

yvonnetangsu
Copy link
Member

@yvonnetangsu yvonnetangsu commented Aug 4, 2020 •
edited
Loading

READY FOR REVIEW

Summary

  • Use the nested sass map approach for modular spacing allowing for easier finetuning and updating of individual values.
  • New sass map $su-responsive-spacing-map in variables/core/_responsive-spacing.scss
  • Add new mixin @responsive-spacing instead of replacing the old one per Decanter meeting discussion.
  • Consider removing the negative modular spacing steps, since the values are 1px apart between adjacent device sizes. Maybe no need for modular spacing at those small ends (just pick the middle value for all devices)?
  • This PR also includes some helper functions, e.g., a "map-deep-get" function that CSS Tricks has featured to retrieve values from nested sass maps.

Needed By (Date)

  • When does this need to be merged by?

Urgency

  • How critical is this PR?

Steps to Test

  1. Pull this branch
  2. Apply @include responsive-spacing('padding', 4 3 null 10); for example to a component
  3. Apply @include responsive-spacing('margin', 8); for example to a component
  4. Verify that it's pulling from the new responsive spacing value in the below sass map (it should render 3 set of spacing values for the 3 breakpoint ranges - xs-sm, md-xl (768px+), 2xl (1500px+))
    Screen Shot 2020-10-08 at 5 14 04 PM

Affected Projects or Products

  • Does this PR impact any particular projects, products, or modules?

Associated Issues and/or People

See Also

@yvonnetangsu
Proof of concept; created nested map for modular spacing values inste…
d52ea19 
…ad of using arbituary multipliers; add helper map-deep-get function to retrieve values from nested sass maps; modular-padding mixin is a temporary one for me to test this way of retrieving spacing
@sherakama sherakama temporarily deployed to Tugboat October 3, 2020 07:32 Destroyed
@sherakama sherakama temporarily deployed to Tugboat October 3, 2020 07:54 Destroyed
@sherakama sherakama temporarily deployed to Tugboat October 3, 2020 20:46 Destroyed
@sherakama sherakama temporarily deployed to Tugboat October 5, 2020 04:54 Destroyed
@yvonnetangsu
Copy link
Member Author

@katrialesser @sherakama This is ready for the two of you to take a look when you have time. I'm not replacing the old @modular-spacing since it's used in so many places, instead, just created @modular-spacing-new which pulls from the new values. Let me know what you think.

@sherakama sherakama temporarily deployed to Tugboat October 5, 2020 21:50 Destroyed
@sherakama sherakama temporarily deployed to Tugboat October 6, 2020 05:39 Destroyed
@sherakama sherakama temporarily deployed to Tugboat October 9, 2020 00:05 Destroyed
@yvonnetangsu
Copy link
Member Author

yvonnetangsu commented Oct 9, 2020 •
edited
Loading

@sherakama @katrialesser Per group discussion today, I'm not replacing the current @modular-spacing mixin, instead created a new one @responsive-spacing. Since we're not replacing the old one, I'm eliminating step -3 and -4 (which we never used anyway and doesn't make much sense to me since the pixel values are basically the same for all breakpoints). No rush on this - just a FYI to take a look when you have a chance. Thanks!

Adding @jenbreese as well since this affects everyone, and @kerri-augenstein to confirm the new spacing values.

@yvonnetangsu
Copy link
Member Author

@sherakama Kerri has confirmed the spacing values. You think we could merge this in and punch a release? I could use this for the Giving site 😄
The cc issues are with the sass map spacing - I kept it consistent with our other sass maps.

@sherakama sherakama temporarily deployed to Tugboat November 4, 2020 18:31 Destroyed
@github-actions github-actions bot added size/xl and removed size/m labels Nov 4, 2020
@sherakama sherakama temporarily deployed to Tugboat November 4, 2020 19:15 Destroyed
@sherakama
Copy link
Member

@yvonnetangsu I've added the deprecated notices and changed modular-scale to responsive-scale everywhere it was used. Please have a review.

@sherakama sherakama temporarily deployed to Tugboat November 4, 2020 19:28 Destroyed
@yvonnetangsu
Copy link
Member Author

@yvonnetangsu I've added the deprecated notices and changed modular-scale to responsive-scale everywhere it was used. Please have a review.

Awesome - thanks for the work. I'll review a bit later today.

@sherakama sherakama temporarily deployed to Tugboat November 4, 2020 19:56 Destroyed
sherakama
sherakama approved these changes Nov 5, 2020
View reviewed changes
Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GTG

@yvonnetangsu
Copy link
Member Author

GTG

Thanks for your work and review! Yay 🥳

@yvonnetangsu yvonnetangsu merged commit 3046778 into master Nov 5, 2020
@yvonnetangsu yvonnetangsu deleted the modular-spacing-map branch November 5, 2020 20:29
yvonnetangsu added a commit that referenced this pull request Jan 13, 2021
@yvonnetangsu
Merge branch 'master' into 611-new-colors
b25311e 
* master:
  Back to dev.
  6.2.1
  D8CORE-2529: changing the condition that makes the link not appear (#775)
  Back to dev.
  Different way of setting up modular spacing (#704)

# Conflicts:
#	DEPRECATIONS.md
#	core/dist/css/decanter.css
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sherakama sherakama sherakama approved these changes

@katrialesser katrialesser Awaiting requested review from katrialesser

@jenbreese jenbreese Awaiting requested review from jenbreese

Assignees
No one assigned
Labels
size/xl
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yvonnetangsu @sherakama