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

[v4] Controls (Button, TextInput, etc.) preset heights / sizes #221

Closed
jeroenransijn opened this issue Jul 12, 2018 · 6 comments
Closed

Comments

@jeroenransijn
Copy link
Contributor

Evergreen is born very implicitly, for example, you can pass a height to a control such as as Button and it works, since it implements a Box under the hood. This gives enormous flexibility, as you run into new use case you haven't encountered yet, you can easily support that use case. Although this was true when Evergreen was born, we have learned much along the way, and common sizes and patterns have become more clear.

Benefits of explicit heights

The benefit of explicit heights for controls is that it's as explicit as you can get. When you do define presets, rarely do you redefine them later (and if you do, it's common you will have to do visual QA, so it could be considered a breaking change). I see the main reason for having presets making it easier for individuals to make decisions, instead of having to rely on passing a value and conforming to 4/8px grid manually.

That's why we have been thinking of giving a bit more opinion to the sizes and heights of controls.

Option 1: Application Level Control

The easiest solution that wouldn't require any change in Evergreen is application level control. The names of the sizes is just an example.

import { Button } from 'evergreen-ui'
import { Sizes } from 'constants'

<Button height={Sizes.SMALL} />
<Button height={Sizes.DEFAULT} />
<Button height={Sizes.LARGE} />

Pros

  • No change in Evergreen
  • Complete control application side

Cons

  • How does this work for the default value?
  • A lot of overhead to keep importing Sizes, and might be tricky

Option 2: Overload the height property with presets.

With Evergreen v4 we introduced theming. In a lot of cases that means properties can access a theme preset, or simply return the value provided. For example: <Pane background="tint1" /> uses a preset, and <Pane background="white" /> simply is passed straight to the component as white.

What this could mean for controls is that we allow themed presets:

theme.getControlHeight = heightOrPreset => {
   if (preset === 'small') return 24

   // In some legacy place we need to be compatible with 30 for whatever reason.
   // This is not the case for Segment, but could help other team adopt Evergreen easier.
   if (preset === 'legacy') return 30

   if (preset === 'default') return 32
   if (preset === 'large') return 40

   return heightOrPreset
}

While using the component this would work as follows:

<Button height="small" />
<Button height="large" />

// Custom height for a one of situation.
<Button height={20} />

Pros

  • Can define heights / sizes on a theme level.
  • Flexibility when you need to break out of this.

Cons

  • Do you really want to support one-of situations?
  • Might need to define a couple of sizes that are required by Evergreen to allow consistency within consumers.

Option 3: Deprecate height. Embrace size.

The final option is very much inline with option 2. Excepts deprecates height completely. You can only add presets defined by the theme.

A. Use strings

<Button size="small" />
<Button size="large" />

Pros

  • Simple.
  • Inline with other component libraries

Cons

  • More prescriptive when there is a requirement for a one-of case.
    • Although the theme can support more sizes. It can become less explicit over time: e.g. the infamous smedium.
  • Loses a tiny bit of explicitness.

Opinion: Leaning towards option 3. Embrace size.

Evergreen is growing up, and we need to make the system more explicit over time. Become more opinionated and get efficiency in return. We embrace size in other parts of our system. For example for typography. So I wouldn't mind embracing the size property and deprecating height altogether for controls. size also seems more consistent with other systems and will allow us to simplify theming logic a bit as well.

The one thing we have to figure out is the perfect scale for sizes. Here is a brain dumb of values (and what I think when I see them) that could be composed in a system:

Some options to compose a scale

  • tiny, very tiny, seems like this should be the smallest? 20
  • small, small size, but is the smallest? 24
  • compact, compact size, but is the smallest? 24
  • default, default size. 32/36
  • medium, medium size, might be confusing when used with default. 32/36/40
  • longform, what the hell is this? Potentially for longform forms. More functional than descriptive. 36
  • big/large, big size, but is the biggest? 36/40/48

Final Proposition

size="small | default | large" => 24, 32, 36

I think those sizes would work well within the scope of our application at Segment. In our product we rarely go up to 40 for controls. But 36 is useful for longer forms. We also use 28 in some places, but I think we can deprecate those to 32. This would be the default values supported in the default theme within Evergreen. Custom themes can still embrace different sizes, or change the meaning of our scale.

@mshwery
Copy link
Contributor

mshwery commented Jul 13, 2018

Have we considered introducing a new size prop to the API that allows you to pass in string aliases (presets) for theme-defined heights without deprecating the height prop?

I think this would be useful. We may want to decouple height from affecting other css attributes though (how does height work when dealing with a Text or Heading component? Or when dealing with a Pane? It only affects the height css attribute. That way consumers that need to explicitly control all the Box properties can still do so, but that's not directly part of the Button or TextInput prop documentation.

size could be our library-wide API for managing sets of size-related css properties on a component. We could predefine some sizes like you suggest (e.g. compact, default, emphasized or large).

There is one caveat: we already have a size API for typography components and it doesn't use string aliases, but instead relies on an nth-100th scale [100, 200, ..., 800, 900]. Would it be too confusing to manage size-related properties (font-size, height, padding, line-height) across all components in a more uniform way? For instance, form controls often can get by with just 2 or 3 variations in size, but typography is a bit more nuanced.

@mshwery
Copy link
Contributor

mshwery commented Jul 13, 2018

FWIW I think we could by with just 3-4 sizes (at least for form controls):

const themeSizes = {
  // smallest native preset, if you need more presets create them in your theme!
  compact: {
    fontSize: '12px',
    height: '24px'
  },
  // aptly named... the default size for all the things
  default: {
    fontSize: '13px',
    height: '36px'
  },
  // or just 'large'
  bigMcLargeHuge: {
    fontSize: '16px',
    height: '48px'
  }
}

And if you are crazy:

const themeSizes = {
  // ...
  jumbo: {
    fontSize: '18px',
    height: '56px'
  }
}

@emadabdulrahim
Copy link

My initial vote goes for Option 2: Overload the height property with presets.

One of the strongest points that Evergreen has is that it's built on top of ui-box. Knowing that, It gives me confidence that I could customize my component the way I want using the naming convention of all supported CSS properties.

Having defaults and string aliases is also very important for quick prototyping, especially if the defaults are carefully put together that covers most use cases.

I've been using Evergreen to prototype an app at work and we are soon pushing it to production. My team was quite surprised how fast I was able to have pixel perfect layout and dashboard. Mostly because of that flexibility that Evergreen has right now. I wouldn't want to lose that.

@jeroenransijn
Copy link
Contributor Author

@mshwery I think it's fine if the size property is a bit different for typography / heading styles. I would be curious as to how we can implement a size and height prop side by side.

Decoupling height from size seems like an interesting approach, we then have to return all props from the size getter that is needed for the Button, which is a bit more than just height and fontSize.

{
  fontSize: '..',
  lineHeight: '..',
  height: '..',
  iconSize: '..',
  iconSpace: '..', // space between icon and label
  paddingX: '..',
}

We could also do some more automatic stuff to derive those properties.

@bmcmahen
Copy link
Contributor

I like the idea of using a size prop with a few predefined sizes, but I don't think this necessarily precludes the use of a height prop.

To overload height and have it alter font sizes, line-heights, padding, and so on, seems a bit confusing and inconsistent. In my mind, height should only alter the height of a button, just as it alters the height of a UI-box. I'd argue the same for input boxes. I've actually been composing both buttons and inputs in my code to basically provide a size prop in an effort to ensure I use either 2-3 different sizes.

My bias to using size might be a legacy of using other ui libraries like bootstrap in the past, but this familiarity has its advantage too -- lots (almost all?) ui libraries use a size prop to denote changes to height, font-size, padding, etc. height usually only adjusts the height of an element.

Ideally you could do something like this:

<Button size='large' height='2rem'>Hello World</Button>

Height would override the default height of a large button.

Alternatively, you could go all in with composition and do something like this:

const LargeButton = ({children}) => (
  <Button height='2rem' fontSize='18px'>
    {children}
  </Button>
)


JustAboutJeff added a commit to JustAboutJeff/evergreen that referenced this issue Aug 4, 2018
JustAboutJeff added a commit to JustAboutJeff/evergreen that referenced this issue Aug 6, 2018
JustAboutJeff added a commit to JustAboutJeff/evergreen that referenced this issue Aug 9, 2018
Rowno added a commit that referenced this issue Sep 24, 2018
* master: (25 commits)
  Remove storybook-deployer
  v3.2.7
  Fix typo in Tooltip proptypes (#321)
  fix githubLink pathname on ComponentReadme (#310)
  Use `rm -rf` in prepublishOnly
  Run the babel builds concurrently
  circleci: fix the gh-pages ignore config
  circleci: fix the gh-pages ignore
  Migrate to circleci 2.0
  Adding a link to Synapse
  Fix wording in Toaster docs (#294)
  v3.2.6
  allow default TableRow keypress events (#221) (#276)
  Fix misspelling of ListItem component name (#268)
  🌲 CI status image (#263)
  Add link to v4 docs and PR
  Add more spacing
  Add Hero image
  Add GitHub hero
  Add note to v4
  ...

# Conflicts:
#	README.md
#	docs/src/components/ComponentReadme.js
#	package.json
#	src/segmented-control/src/SegmentedControl.js
#	src/segmented-control/src/SegmentedControlRadio.js
#	src/segmented-control/src/styles/SegmentedControlAppearances.js
#	src/select-menu/src/OptionsList.js
#	src/select-menu/src/SelectMenuContent.js
#	src/table/src/TableRow.js
#	src/table/stories/index.stories.js
#	src/toaster/docs/index.js
#	yarn.lock
Rowno added a commit that referenced this issue Sep 25, 2018
* v4: (30 commits)
  Tracking fix (#338)
  add segment tracking (#337)
  Upgrade dependencies v4 (#336)
  V4  Docs (#335)
  Remove storybook-deployer
  Remove storybook-deployer
  v3.2.7
  Fix typo in Tooltip proptypes (#321)
  fix githubLink pathname on ComponentReadme (#310)
  Use `rm -rf` in prepublishOnly
  Run the babel builds concurrently
  circleci: fix the gh-pages ignore config
  circleci: fix the gh-pages ignore
  Migrate to circleci 2.0
  Adding a link to Synapse
  Fix wording in Toaster docs (#294)
  v3.2.6
  allow default TableRow keypress events (#221) (#276)
  Fix misspelling of ListItem component name (#268)
  🌲 CI status image (#263)
  ...

# Conflicts:
#	package.json
@jeroenransijn jeroenransijn mentioned this issue Oct 18, 2018
2 tasks
@stale
Copy link

stale bot commented Mar 11, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wont-fix label Mar 11, 2019
@mshwery mshwery closed this as completed Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants