Skip to content

Conversation

@siddharthkp
Copy link
Contributor

Grid + Stack

Built on top of #3269

@siddharthkp siddharthkp requested a review from SaraVieira January 7, 2020 15:32
@siddharthkp siddharthkp added the UI label Jan 7, 2020
@lbogdan lbogdan temporarily deployed to pr3277 January 7, 2020 15:39 Inactive
@lbogdan
Copy link
Contributor

lbogdan commented Jan 7, 2020

Build for latest commit 2091157 is at https://pr3277.build.csb.dev/s/new.

// valid combinations are
// start | start + end | start + span | span
// span + end is also possible but not implemented here
export const Column = styled.div<{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const Column = styled.div<{
type ColumnEnd = number | number[];
type ColumnSpan = number | number[];
type ColumnStart = number | Array<number | string>;
type ColumnProps =
| { end: ColumnEnd; start: ColumnStart; }
| { span: ColumnSpan; }
| { span: ColumnSpan; start: ColumnStart; }
| { start: ColumnStart; };
export const Column = styled.div<ColumnProps>(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this improve?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is implementing the Props like you said in your comments 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Implementing it this way would give errors on the missing prop in each, right?

repro (see end prop): https://codesandbox.io/s/funny-pare-zswfv

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • This is not a blocker, so I'm merging the PR but will continue to improve the types

SaraVieira and others added 2 commits January 7, 2020 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants