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

Height aligned 2 buttons in manager's header #1769

Merged
merged 2 commits into from
Aug 30, 2017

Conversation

usagi-f
Copy link
Contributor

@usagi-f usagi-f commented Aug 30, 2017

This PR is nitspick.

The height of the 2 buttons are not aligned in Chrome.
I am a designer, this feel sick to me...

ScreenShot in Chrome

image

ScreenShot in Safari

image

What I did

Using flexbox

After

ScreenShot in Chrome

image

ScreenShot in Safari

image

How to test

Is this testable with jest or storyshots?

Yes, maybe by storyshots.

Does this need a new example in the kitchen sink apps?

No.

Does this need an update to the documentation?

No.


I'm contributing to this repository for the first time.

Sorry, I did not fully understand if there are other files that need to be changed.

@codecov
Copy link

codecov bot commented Aug 30, 2017

Codecov Report

Merging #1769 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1769      +/-   ##
==========================================
+ Coverage   21.14%   21.14%   +<.01%     
==========================================
  Files         252      252              
  Lines        5694     5693       -1     
  Branches      682      689       +7     
==========================================
  Hits         1204     1204              
+ Misses       3992     3962      -30     
- Partials      498      527      +29
Impacted Files Coverage Δ
.../ui/src/modules/ui/components/left_panel/header.js 29.62% <50%> (+1.05%) ⬆️
...es__/update-addon-info/update-addon-info.output.js 0% <0%> (ø) ⬆️
...react-native/src/manager/components/PreviewHelp.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/api/actions/api.js 50.47% <0%> (ø) ⬆️
.../ui/src/modules/ui/components/layout/dimensions.js 15.62% <0%> (ø) ⬆️
addons/knobs/src/components/PropField.js 10.86% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/init_panels.js 25% <0%> (ø) ⬆️
...ponents/left_panel/stories_tree/tree_decorators.js 33.33% <0%> (ø) ⬆️
.../src/manager/containers/CommentsPanel/dataStore.js 34.78% <0%> (ø) ⬆️
...res__/update-addon-info/update-addon-info.input.js 0% <0%> (ø) ⬆️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5aeaa7...100f8cd. Read the comment docs.

@ndelangen
Copy link
Member

@marchdoe Would you like to review this one?

@ndelangen
Copy link
Member

Thank you @usagi-f, awesome PR, very clear what it does and I'm sure we can get this merged real soon and end this atrocity to your eyes. 👨‍🎨

I want to give some new members on the team the opportunity to review and merge for the first time.

@marchdoe
Copy link
Member

@usagi-f - I am trying to recreate the issue and am not able to.

I am on Chrome Version 60.0.3112.113 (Official Build) (64-bit) (MacOS)

screen shot 2017-08-30 at 9 39 56 am

Great work on this! Everything looks great in the PR. The only thing I noticed that I was more curious about than anything was the nesting of Flex. Is it possible to achieve the same display without nesting?

@ndelangen
Copy link
Member

Maybe the issue only appears when zooming?

@usagi-f
Copy link
Contributor Author

usagi-f commented Aug 30, 2017

@marchdoe
Thank you review for this PR.

I also reviewed it in the same environment. (60.0.3112.113 / 64-bit / MacOS)
But, display was same.

  • cra-kitchen-sink
    image

  • Install for "@storybook/react": "^3.2.8" in the new directory
    image


And, Thank you for review on CSS too.

But, I think the nested flexbox is not a bad thing.

By doing this, eliminate the Magic-Number of padding, and The height becomes flexible.

How about you think this...?

@marchdoe
Copy link
Member

@usagi-f - awesome, sounds good to me, lets get this into master!

cc @ndelangen

@Hypnosphi Hypnosphi self-requested a review August 30, 2017 16:59
Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

Looks awesome to me

@ndelangen
Copy link
Member

@marchdoe you can press 'Update branch' and then wait for the CI to finish and then press 'Merge' 😄👍

@marchdoe marchdoe merged commit d37ae4c into storybookjs:master Aug 30, 2017
@ndelangen
Copy link
Member

Congrats on your first merge as maintainer 👍

@usagi-f
Copy link
Contributor Author

usagi-f commented Aug 31, 2017

Thanks to everyone of the maintainers! 😆

I hope the storybook will be a valuable product!

@shilman shilman mentioned this pull request Aug 31, 2017
@ndelangen ndelangen changed the title Height aligned the 2 button Height aligned 2 button in manager's header Sep 1, 2017
@ndelangen ndelangen changed the title Height aligned 2 button in manager's header Height aligned 2 buttons in manager's header Sep 1, 2017
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.

4 participants