-
Notifications
You must be signed in to change notification settings - Fork 829
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
Comments
Have we considered introducing a new I think this would be useful. We may want to decouple
There is one caveat: we already have a |
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'
}
} |
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. |
@mshwery I think it's fine if the Decoupling
We could also do some more automatic stuff to derive those properties. |
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 Ideally you could do something like this:
Height would override the default height of a large button. Alternatively, you could go all in with composition and do something like this:
|
* 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
* 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
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. |
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 aBox
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.
Pros
Cons
Sizes
, and might be trickyOption 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 aswhite
.What this could mean for controls is that we allow themed presets:
While using the component this would work as follows:
Pros
Cons
Option 3: Deprecate
height
. Embracesize
.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
Pros
Cons
smedium
.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 deprecatingheight
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
20
24
24
32
/36
default
.32
/36
/40
36
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 use28
in some places, but I think we can deprecate those to32
. This would be the default values supported in the default theme withinEvergreen
. Custom themes can still embrace different sizes, or change the meaning of our scale.The text was updated successfully, but these errors were encountered: