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

In spacing should be same naming as in fonts #2308

Closed
ryskin opened this issue Nov 17, 2022 · 6 comments · Fixed by #2433
Closed

In spacing should be same naming as in fonts #2308

ryskin opened this issue Nov 17, 2022 · 6 comments · Fixed by #2433

Comments

@ryskin
Copy link

ryskin commented Nov 17, 2022

That's really confusing in one place use extraLarge in another place it's just xl. I think naming is much more intuitive in fonts.

image

image

I propose adding more x ) like xxxs and xxxl, for extra small and extra big sizes.

The current demo really is the best of all time.

@yulolimum
Copy link
Member

I like this proposal. I get confused between micro/tiny and huge/massive all the time. @robinheinze what do you think?

@jamonholmgren
Copy link
Member

I don't have an opinion on which one is better, but I agree they should be consistent.

BTW, the as TextStyle doesn't do actual type-checking, just coerces/casts it as that.

CleanShot 2022-11-16 at 19 22 39@2x

I seem to remember some way to make a generic that would actually type-check when used as Only<Thing> or something, though?

@yulolimum
Copy link
Member

yulolimum commented Nov 17, 2022

@jamonholmgren

It doesn't do typechecking but it does typeahead:
yulolimum-capture-2022-11-16--19-34-39

You can do Record generic...
yulolimum-capture-2022-11-16--19-35-36

...but you lose the key union type that we have higher up in the component:
yulolimum-capture-2022-11-16--19-37-23

If you have a way to do both - would be great!

Let's open a new issue for that if you want.

@jamonholmgren
Copy link
Member

Opened #2309 for that.

@robinheinze
Copy link
Member

robinheinze commented May 5, 2023

I'm totally fine if we want to change spacing scale to match the text size scale.

export const spacing = {
  xxxs: 2,
  xxs: 4,
  xs: 8,
  sm: 12,
  med: 16,
  lg: 24,
  xl: 32,
  xxl: 48,
  xxxl: 64,
} as const

@infinitered-circleci
Copy link

🎉 This issue has been resolved in version 8.8.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

6 participants