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

[BUG] Style: Title of Panels #959

Closed
wcastand opened this issue Apr 25, 2017 · 7 comments
Closed

[BUG] Style: Title of Panels #959

wcastand opened this issue Apr 25, 2017 · 7 comments

Comments

@wcastand
Copy link
Contributor

Hi,

I'm developing an addon for storybook and i create a panel with a custom React component as title and the title aren't centered with flex so it doesn't look really nice.

screenshot

Is it possible to center the Panel's titles or he can break something i'm not aware of ?

If it's possible, i'd love to help and do a PR :)

@usulpro
Copy link
Member

usulpro commented Apr 26, 2017

Hi @wcastand!

Thank you for the great addon!
You reported the cool feature, wich I guess didn't mention in our docs. How do you think should we add this?

The place you want to fix is here. But before creating PR did you consider the possibility that the issue is on your addon side? Because when I tried to reproduce it in a single example:

title: () => (<div>The Title</div>)

it looked good and was properly centered:

image

Maybe live demo with your addon could help us to test it and investigate the bug?

@usulpro usulpro closed this as completed Apr 26, 2017
@usulpro usulpro reopened this Apr 26, 2017
@wcastand
Copy link
Contributor Author

wcastand commented Apr 26, 2017

Hi !

Thank you for storybook !! :)
Yeah it's not mentionned that title can be a react component but i test and it worked so i was happy.

I think my addon break the layout of the titles because he contains a button and he's larger than other title.

I use Chrome on OSX (Just in case)
I try to add align-items: center; on the container div and it work pretty well.
I'm not 100% sure but i think the align-items: center; should be added to style.tabbar here

capture d ecran 2017-04-26 a 14 22 55

Right now, no align-items is specified so he take the default which is flex-start

For the documentation about the possibility of using a React component in the title. I think we can simply add it in the example of addonAPI.addPanel() here

Something like that should be enough i think :

addonAPI.register('kadira/notes', (storybookAPI) => {
  // Also need to set a unique name to the panel.
  addonAPI.addPanel('kadira/notes/panel', {
    title: () => <span>Notes</span>,
    render: () => (
      <Notes channel={addons.getChannel()} api={storybookAPI}/>
    ),
  })
})

Or maybe something more complete in case people doesn't know stateless component in react ?

class Title extends React.Component {
  render(){
    return <span>Notes</span>;
  }
}

addonAPI.register('kadira/notes', (storybookAPI) => {
  // Also need to set a unique name to the panel.
  addonAPI.addPanel('kadira/notes/panel', {
    title: () => <Title/>,
    render: () => (
      <Notes channel={addons.getChannel()} api={storybookAPI}/>
    ),
  })
})

@usulpro
Copy link
Member

usulpro commented Apr 26, 2017

Yeap @wcastand I got your point!

I think PR for fixing that would be very welcomed!
At the same time, how do you think is it worth to add some reasonable limit of max-height?

@wcastand
Copy link
Contributor Author

I'll do a PR tonight then :)

For the max-height, it could be a good idea. Or maybe we fix the height and put a align-items: stretch. That way all the title will be the same height and centered properly and the developer know which height it can use.

Let me know if you prefer a max-height or fixed height with a align-items: stretch and which max height you think would be enough. A max-height of 60px seems good and allow the developer some freedom in what he can put in the title (like my addon which add a copy button).

@usulpro
Copy link
Member

usulpro commented Apr 26, 2017

align-items: stretch lefts tubs stuck to the top of panel, so max-height and overflow: hidden in tablink seems a better solution. Agree 60px would be quite enough

@wcastand
Copy link
Contributor Author

Ok, i'll make the Pr with the max-height solution.

ndelangen added a commit that referenced this issue Apr 28, 2017
#959 add a max-height and center element with alignItems: center
@usulpro
Copy link
Member

usulpro commented Apr 29, 2017

Fixed by #961

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants