Skip to content
This repository was archived by the owner on Jan 9, 2023. It is now read-only.

✨ tabs components#97

Merged
bpetetot merged 4 commits into
masterfrom
tabs
Jul 13, 2017
Merged

✨ tabs components#97
bpetetot merged 4 commits into
masterfrom
tabs

Conversation

@bpetetot
Copy link
Copy Markdown
Contributor

The <Tabs> component will be used in the workbench screen (for props, state...) and the documentation screen (to switch from edit to preview mode)

It's a basic implementation of tabs, that can be used like that :

<Tabs className="myClass">
    <Tab title="props"><Props /></Tab>
    <Tab title="state">Not implemented yet.</Tab>
</Tabs>

The following features are available on the <Tabs> component

  • Tabs#defaultKey prop to select a default tab
  • Tabs#actions prop to add an action bar on the right of the tabs
  • Tab#title prop is a PropTypes.node

And the Tabs, Tab, TabsHeader and TabsActions styles can be overridden.

@bpetetot bpetetot requested a review from fabienjuif July 12, 2017 14:45
Comment thread src/gui/components/tabs/tabs.styles.js Outdated

const layout = css({
position: 'relative',
border: '1px solid #ddd',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

em

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ah... I'm not agree with this one.
I don't want to have a border of '1.333px' if I pass my font from '10px' to '13px'.
I want to have a constant size of '1px' here on the border.

Copy link
Copy Markdown
Contributor

@fabienjuif fabienjuif Jul 12, 2017

Choose a reason for hiding this comment

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

this not only about user font size.
this is also about making the component reusable.

if you want your tabs to be big, you just have to use them and change font-size with the em hack :
<Tabs style={{ fontSize: '2em' }} />

==> I have tabs that are 2 times bigger, border included (em) or not (px).

Copy link
Copy Markdown
Contributor Author

@bpetetot bpetetot Jul 12, 2017

Choose a reason for hiding this comment

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

ok I understand, but how do you compute the value in em of your border size in your component ? you put 0.1em because it seems low ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some times I have 0.02em

Navbar.defaultProps = {
style: {},
className: '',
className: undefined,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we change all those defaultProps ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

because in glamor#merge, falsy values are : false, null, undefined and {}.
'' is not considered as falsy.


class Tabs extends Component {
state = {
selected: this.props.defaultKey,
Copy link
Copy Markdown
Contributor

@fabienjuif fabienjuif Jul 12, 2017

Choose a reason for hiding this comment

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

I really don't like that pattern.

  • we handle state in graphical components
  • parent give callback, + key to child that call the callback with key as parameter
  • we have a key, this is title we don't need to use index IMO.

at least, can't we create a curry version of handleSelectedTab ?

handleSelectedTab = (key) => () => {
  //...
}

// ...

titles={titles.map(title => <li {/* ... */} onClick={handleSelectedTab(title)}>{...}</li>)}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I know that you don't like inner state 😇

Otherwise, I give the index in that case, because title is a PropTypes.node (for example if I want to have an icon + label in my tab header). So titlecan't be a key, maybe I should add a Tab#tabKey prop.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#99

@bpetetot bpetetot merged commit 6468f39 into master Jul 13, 2017
@bpetetot bpetetot deleted the tabs branch July 13, 2017 20:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants