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

Generalize toolbox box component - Closes #2313 #2338

Merged
merged 23 commits into from Aug 14, 2019

Conversation

@slaweet
Copy link
Member

commented Aug 9, 2019

What issue have I solved?

#2313

How have I implemented/fixed it?

Improvements to the Box component

  • Create <Box.Header /> <Box.Footer /> <Box.Content /> components
  • Create <Box.EmptyState /> component
  • Expose <Tabs /> as <Box.Tabs />
  • Create a new prop: <Box isLoading>
  • Create a new prop: <Box width="medium" /> max-width: 720px
  • Create a new prop: <Box main /> min-height: 600px
  • Create <Box.FooterButton /> component
  • Create <Box.Row /> component (with hover, paddings, and border-bottom gray line)
  • Update BoxDemo to showcase all the options of Box component
  • Update Toolbox demo to show a menu for quick access to each section

There will be a follow-up to update all components #2342

How has this been tested?

Review checklist

@slaweet slaweet self-assigned this Aug 9, 2019

@slaweet slaweet force-pushed the 2313-generalize-toolbox-box-component branch 3 times, most recently from 2838349 to 16cb9b2 Aug 9, 2019

@slaweet slaweet requested review from massao and osvaldovega Aug 12, 2019

@slaweet slaweet added this to Pull Requests in Version 1.20.0 via automation Aug 12, 2019

slaweet added some commits Aug 9, 2019

🔥 Remove 'dark' prop from inputs demo
... because it is not implemented yet and thus was polluting test output
with warnings.

@slaweet slaweet force-pushed the 2313-generalize-toolbox-box-component branch from 82a7d0c to 63861bb Aug 12, 2019

@yasharAyari
Copy link
Member

left a comment

The issue that you working on is too big and general. Please split this into blew tickets and fix each issue in a separated pull request and change the type of the current issue to an epic. So it would be easier for the team in the future to follow the changes that you made.

  • Create <Box.Header /> <Box.Footer /> <Box.Content /> components
  • Create <Box.EmptyState /> component
  • Expose <Tabs /> as <Box.Tabs />
  • Create a new prop: <Box isLoading>
  • Create a new prop: <Box width="medium" /> max-width: 720px
  • Create a new prop: <Box main /> min-height: 600px
  • Create <Box.FooterButton /> component
  • Create <Box.Row /> component (with hover, paddings, and border-bottom gray line)

slaweet added some commits Aug 9, 2019

@slaweet slaweet force-pushed the 2313-generalize-toolbox-box-component branch from 63861bb to 6126b5c Aug 12, 2019

@slaweet

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

@yasharAyari I commits so that this PR is only changing the Box and its sub-components. And there is a follow-up in progress that aims to change other components to use the new Box capabilities. #2343

I think this PR is 3 story points and the next one will be 5 story points, so I'll keep an eye on the size of the follow-up and if it starts growing too much, I'll cut it in half.

Having said that, this PR is ready for review.

@slaweet slaweet requested review from yasharAyari and massao Aug 12, 2019

@slaweet slaweet marked this pull request as ready for review Aug 12, 2019

slaweet added some commits Aug 12, 2019

@massao

massao approved these changes Aug 13, 2019

Copy link
Contributor

left a comment

Good job, thanks Vit 👍

@reyraa reyraa added this to Pull Requests in Version 1.21.0 via automation Aug 13, 2019

@reyraa reyraa removed this from Pull Requests in Version 1.20.0 Aug 13, 2019

@yasharAyari
Copy link
Member

left a comment

@slaweet Please makes sure that from now on you will break big issues like this one to smaller chucks. So it would be easier to track the changes in the future.

@slaweet slaweet merged commit 3e7b11f into development Aug 14, 2019

3 checks passed

Jenkins e2e tests e2e tests passed
Details
Jenkins test deployment Commit was deployed to test
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

Version 1.21.0 automation moved this from Pull Requests to Merged Pull Requests Aug 14, 2019

@slaweet slaweet added the ready label Aug 14, 2019

@slaweet slaweet deleted the 2313-generalize-toolbox-box-component branch Aug 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.