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

Theamble Spacing #967

Open
nfiniteset opened this Issue May 30, 2018 · 5 comments

Comments

5 participants
@nfiniteset
Collaborator

nfiniteset commented May 30, 2018

As a developer
I can put space between elements
so my app looks like the HIG specs


This is a little element that takes up space.


Dynamic sizing by t-shirt size

The spacing prop takes a t-shirt size and uses density-specific values from theme data.

<Spacer spacing="m" /> // 12px at high density, 16px at medium density

Specify size by value

The size prop takes a valid CSS width value.

<Spacer size="16px" /> // always 16px

PropTypes

// somewhere in the @hig/theme-data package
const AVAILABLE_SPACINGS = ['xxs', 'xs', 's', 'm', 'l', 'xl', 'xxl']; // maybe derive these programmatically from `theme-data/src/themes/baseTheme/system/density.js`
// within @hig/spacer/src/stylesheet.js
// Maybe something like this:
function getSpacing(size, themeData) {
  return themeData[`DENSITY.SPACINGS.${size.toUpperCase()}`];
}
// Within @hig/spacer/src/Spacer.js
import { AVAILABLE_SPACINGS } from '@hig/theme-data';

Spacer.propTypes = {
  spacing: PropTypes.oneOf(AVAILABLE_SPACINGS),
  size: PropTypes.string,
  display: PropTypes.oneOf('block', 'inline-block', 'flex')
}

Spacer.defaultProps = {
  spacing: 'm',
  display: 'flex'
}

Dev notes

  • Spacing is a square with width and height derived from props.
  • When both spacing and size are provided, favor size.

@nfiniteset nfiniteset created this issue from a note in HIG backlog (Priorities) May 30, 2018

@eskfung

This comment has been minimized.

Show comment
Hide comment
@eskfung

eskfung Jun 7, 2018

Collaborator

It may not be necessary to recreate the vanilla spacing classes since @hig/styles is now a public package.

Collaborator

eskfung commented Jun 7, 2018

It may not be necessary to recreate the vanilla spacing classes since @hig/styles is now a public package.

@morrisallison morrisallison added the chore label Jun 15, 2018

@nfiniteset nfiniteset changed the title from Revamp Spacing to Theamble Spacing Aug 17, 2018

@halfghaninne halfghaninne moved this from Priorities to In Progress in HIG backlog Sep 4, 2018

@halfghaninne halfghaninne self-assigned this Sep 4, 2018

@halfghaninne

This comment has been minimized.

Show comment
Hide comment
@halfghaninne

halfghaninne Sep 5, 2018

Collaborator

@nfiniteset

Do we expect someone using this component to pass in "m" (or "medium", for that matter) as a hypothetical value of a spacing prop?

If so, I think the PropType might look like:
spacing: PropTypes.oneOf("xxs", "s", "m", "l", "xl", "xxl"),
and then work would be done in stylesheet.js to make a translation to one of the available spacings in the theme-data package, according to the density of whatever ThemeContext (themeClass? Still figuring out the difference) the component has.

Otherwise, the expectation if the PropType is as written:
spacing: PropTypes.oneOf(AVAILABLE_SPACINGS)

...and AVAILABLE_SPACINGS is coming from "@hig/theme-data/src/basics/spacings"

...would be that the user passes props to the component like "LOW_M". Is that right?

Collaborator

halfghaninne commented Sep 5, 2018

@nfiniteset

Do we expect someone using this component to pass in "m" (or "medium", for that matter) as a hypothetical value of a spacing prop?

If so, I think the PropType might look like:
spacing: PropTypes.oneOf("xxs", "s", "m", "l", "xl", "xxl"),
and then work would be done in stylesheet.js to make a translation to one of the available spacings in the theme-data package, according to the density of whatever ThemeContext (themeClass? Still figuring out the difference) the component has.

Otherwise, the expectation if the PropType is as written:
spacing: PropTypes.oneOf(AVAILABLE_SPACINGS)

...and AVAILABLE_SPACINGS is coming from "@hig/theme-data/src/basics/spacings"

...would be that the user passes props to the component like "LOW_M". Is that right?

@nfiniteset

This comment has been minimized.

Show comment
Hide comment
@nfiniteset

nfiniteset Sep 5, 2018

Collaborator

I added some more detail to the story description above.

Collaborator

nfiniteset commented Sep 5, 2018

I added some more detail to the story description above.

@nfiniteset

This comment has been minimized.

Show comment
Hide comment
@nfiniteset

nfiniteset Sep 5, 2018

Collaborator

themeClass is old news. Forget about it.

Everything we build from here on out will pull values from the themeData object provided by the ThemeContext.Consumer.

Collaborator

nfiniteset commented Sep 5, 2018

themeClass is old news. Forget about it.

Everything we build from here on out will pull values from the themeData object provided by the ThemeContext.Consumer.

@halfghaninne

This comment has been minimized.

Show comment
Hide comment
@halfghaninne

halfghaninne Sep 5, 2018

Collaborator

Awesome! Yeah that function in stylesheet was exactly like what I was envisioning.

Collaborator

halfghaninne commented Sep 5, 2018

Awesome! Yeah that function in stylesheet was exactly like what I was envisioning.

@nfiniteset nfiniteset moved this from In Progress to Priorities in HIG backlog Sep 12, 2018

@nfiniteset nfiniteset added the theming label Sep 12, 2018

@wmui51 wmui51 assigned wmui51 and unassigned halfghaninne Sep 17, 2018

@wmui51 wmui51 moved this from Priorities to In Progress in HIG backlog Sep 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment