-
Notifications
You must be signed in to change notification settings - Fork 45
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
Unifies Box/Flex #707
Unifies Box/Flex #707
Conversation
Ah, I see that also hits the Typography component. I'm also thinking we can dramatically simplify that once we fix the font declarations so, maybe that's up next. |
textAlign, | ||
TextAlignProps, | ||
verticalAlign, | ||
VerticalAlignProps, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
verticalAlign
is included in the new layout
helper which we should use instead.
The layout utility includes style props for
width
,height
,display
,minWidth
,minHeight
,maxWidth
,maxHeight
,size
,verticalAlign
,overflow
,overflowX
, andoverflowY
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Likewise, a lot of the other props can be replace with typography
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I figured I'd update Typography on it's own because it can mostly go away with fixing the font-face declarations.
I think the type failures that you're getting here is what I was discussing with Orta. The keys for the typesizes in the theme file are defined as "1", "2", etc but get simplified to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My questions have been answered, so 👍 from me.
Once the type errors have been fixed, feel free to merge.
Hm, ok yeah those errors are specifically related to TypeScript 3.9.3. Is that an actual bug? We can maybe address this when we implement the new typography scale and work out a plan for deprecating the old styles (maybe convert them into non-numeric strings and leave it alone) |
From my conversation w/ @orta it sounded like it was intentional (because it's fundamentally equivalent to TS under the hood). We could get around it by broadening the typography props to accept any string. |
oh cool - Damon's back, hiiiii |
@dzucconi - so to be clear, this just collapses the shared code and you suggest we continue using |
I don't think this matters much. I'd probably exclusively use |
So looking at force/eigen. There's 3 places in each that use the flexGrow="string" syntax which would no longer be supported. Simple fix to upgrade for each but technically this release would be a breaking change. |
🚀 PR was released in |
Picking up on the initial impetus behind #704
We might have a few minor issues surrounding the flexGrow/Shrink props (which are now required to be numeric). I noticed that since
size
was being passed to the underlying Flex in Avatar, that was causing an issue because space ships with asize
mixin which sets width/height. Maybe we just remove that, actually?📦 Published PR as canary version:
10.1.1-canary.707.11364.0
✨ Test out this PR locally via: