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

Add spacing variables #44

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

AlejandroPerezMartin
Copy link
Contributor

Description of the Change

This PR adds CSS spacing variables.

Benefits

Have consistent spacing and improve maintainability.

Verification Process

Add CSS selector and use any of the variables, for example:

section {
  margin-bottom: var(--s-large);
  margin-top: var(--s-xsmall);
}

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog Entry

  • ADDED: CSS Spacing Variables

@barneyjeffries
Copy link

@AlejandroPerezMartin Can we use more verbose var naming? Prefer --spacing-large

cc @joesnellpdx

@AlejandroPerezMartin
Copy link
Contributor Author

AlejandroPerezMartin commented Jul 29, 2021

@AlejandroPerezMartin Can we use more verbose var naming? Prefer --spacing-large

cc @joesnellpdx

Totally agree @barneyjeffries, pushed a commit renaming those variables, thanks!

@AlejandroPerezMartin AlejandroPerezMartin marked this pull request as ready for review August 2, 2021 12:19
@barneyjeffries
Copy link

@AlejandroPerezMartin This looks good to me now!

@devinle
Copy link

devinle commented Feb 25, 2022

@AlejandroPerezMartin I'm wondering how you would feel about using an 8pt system, rather than 10? I'm not opposed either way, I've just had a good deal of success working with designers in an 8pt grid.

@joesnellpdx what do you think?

:root { --spacing-xxsmall: 0.5rem; /* 8px */ --spacing-xsmall: 1rem; /* 16px */ --spacing-small: 2rem; /* 32px */ --spacing-medium: 3rem; /* 48px */ --spacing-xmedium: 4rem; /* 64px */ --spacing-xxmedium: 5rem; /* 80px */ --spacing-large: 6rem; /* 96px */ --spacing-xlarge: 7rem; /* 112px */ --spacing-xxlarge: 8rem; /* 128px */ }

@joesnellpdx
Copy link
Contributor

joesnellpdx commented Feb 25, 2022

@AlejandroPerezMartin I'm wondering how you would feel about using an 8pt system, rather than 10? I'm not opposed either way, I've just had a good deal of success working with designers in an 8pt grid.

@joesnellpdx what do you think?

:root { --spacing-xxsmall: 0.5rem; /* 8px */ --spacing-xsmall: 1rem; /* 16px */ --spacing-small: 2rem; /* 32px */ --spacing-medium: 3rem; /* 48px */ --spacing-xmedium: 4rem; /* 64px */ --spacing-xxmedium: 5rem; /* 80px */ --spacing-large: 6rem; /* 96px */ --spacing-xlarge: 7rem; /* 112px */ --spacing-xxlarge: 8rem; /* 128px */ }

@AlejandroPerezMartin @devinle

First, I'd run it by Lea - to see if VisD has a preference if they had to pick.

I'd go with 8px for general spacing, but use a default VAR to set it.

So, have a default var like this:

:root {
   --spacing-base: .5rem; // 8px
}

:root {
--spacing-half-x: calc(var(--spacing-bas) * .5); // 4px
--spacing-2-x: calc(var(--spacing-base) * 2); // 16px
--spacing-3-x: calc(var(--spacing-base) * 3); // 24px
--spacing-4-x: calc(var(--spacing-base) * 4); // 32px
...
}

For font scale - we may want to default with something based on a type scale - but, again, I'd see what Lea says.

CC: @dmtrmrv

@rdimascio
Copy link
Contributor

These variables should probably be added to theme.json, yeah? If so, I opened a PR for it here — #93, if not I'll close it out.

@joesnellpdx
Copy link
Contributor

These variables should probably be added to theme.json, yeah? If so, I opened a PR for it here — #93, if not I'll close it out.

@devinle Per @rdimascio comment / pr above - we should determine our next steps and dovetail with our UI Plugin approach.

Once we get that in place, we should get reviews from those here and close the other issues / PRs.

@joesnellpdx joesnellpdx mentioned this pull request Mar 10, 2022
6 tasks
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

Successfully merging this pull request may close these issues.

5 participants