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

Unifies Box/Flex #707

Merged
merged 2 commits into from
Jun 15, 2020
Merged

Unifies Box/Flex #707

merged 2 commits into from
Jun 15, 2020

Conversation

dzucconi
Copy link
Member

@dzucconi dzucconi commented Jun 3, 2020

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 a size 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:

npm install @artsy/palette@10.1.1-canary.707.11364.0
# or 
yarn add @artsy/palette@10.1.1-canary.707.11364.0

@dzucconi dzucconi added the canary Publish a canary for this PR label Jun 3, 2020
@dzucconi dzucconi requested review from damassi and zephraph June 3, 2020 19:28
@dzucconi
Copy link
Member Author

dzucconi commented Jun 3, 2020

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,
Copy link
Contributor

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, and overflowY.

https://styled-system.com/api#layout

Copy link
Contributor

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 )

https://styled-system.com/api#typography

Copy link
Member Author

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.

@zephraph
Copy link
Contributor

zephraph commented Jun 3, 2020

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 1, 2, etc automatically by TS. I'm not sure exactly what to do about that yet.

Copy link
Contributor

@zephraph zephraph left a 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.

@dzucconi
Copy link
Member Author

dzucconi commented Jun 3, 2020

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)

@zephraph
Copy link
Contributor

zephraph commented Jun 3, 2020

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.

@orta
Copy link
Contributor

orta commented Jun 3, 2020

oh cool - Damon's back, hiiiii

@damassi
Copy link
Member

damassi commented Jun 6, 2020

@dzucconi - so to be clear, this just collapses the shared code and you suggest we continue using <Flex> and <Box> for markup clarity, yah? Or just <Box>?

@dzucconi
Copy link
Member Author

dzucconi commented Jun 8, 2020

we continue using <Flex> and <Box> for markup clarity, yah? Or just <Box>

I don't think this matters much. I'd probably exclusively use <Box>. I suppose there's something to be said about components that are shared with iOS and having to exclusively use <Flex> for those?

@dzucconi
Copy link
Member Author

dzucconi commented Jun 8, 2020

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.

@dzucconi dzucconi assigned damassi and unassigned dzucconi Jun 8, 2020
@dzucconi dzucconi merged commit 115d48e into master Jun 15, 2020
@artsyit
Copy link
Contributor

artsyit commented Jun 15, 2020

🚀 PR was released in v11.0.0 🚀

@damassi damassi deleted the feat/flex-box branch June 15, 2020 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
canary Publish a canary for this PR released Version: Major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants