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

feat(Tab): Add component #430

Merged
merged 2 commits into from
Jun 23, 2017
Merged

feat(Tab): Add component #430

merged 2 commits into from
Jun 23, 2017

Conversation

levithomason
Copy link
Member

@levithomason levithomason commented Aug 24, 2016

Fixes #199. This PR adds the Tab component. There are a number of ways we could go with the API. I'll post an API proposal soon.

Considering a Tab component is simply a tabular menu followed by some tab segments, I think we may create very a simple Tab API. More complicated use of Tabs could use an actual Menu and Segments with manual wiring of the click handlers and active class.

Simple Tabs

image

<Tab>
  <Tab.Pane name='First'>
    First
  </Tab.Pane>
  <Tab.Pane name='Second'>
    Second
  </Tab.Pane>
  <Tab.Pane name='Third'>
    Third
  </Tab.Pane>
</Tab>

Custom Tabs

This type of customization requires a full definition of the Menu component (icons, right sub menu, etc). I think at this point, the user should just wire in their click handlers to render tab Segments manually. An API for doing this will make the simple use case obtuse.

Open to suggestions on how to accomplish both with a clean and concise API.

image

Considerations

This component doesn't lend itself well to an array shorthand prop such as panes since the content is bound to be much more than just text. Because of this children will be used often. It is difficult and unreliable to loop/filter children to add click handlers and props for auto controlling things like the active Segment (pane). We are also avoiding React context to do so if at all possible.

Given these points, I'm leaning toward something like the simple tabs markup shown above. Will come back to this later. Feedback is much welcomed.

@jeffcarbs
Copy link
Member

jeffcarbs commented Aug 24, 2016

I just implemented a simple tab-based component in our app. Keep track of the selected tab by it's array index so you can easily determine which tab should be active and which pane should be visible. The paneProps could either be an as element via the component augmentation API or you could define a renderer like the Table API. Would look roughly like:

const tabs = [
  { 
    ...tabProps,
    pane: {
      ...paneProps
    }
  }
]

<Tab tabs={tabs} />

// Would render:

<Tab.Menu>
  {tabs.map(tab, (props, index) => <Tab.Menu.Item {...props} active={index === this.state.selectedIndex} /> */}
</Tab.Menu>
{<Tab.Pane {...tabs[this.state.selectedIndex].paneProps} />  */}

@levithomason
Copy link
Member Author

Thanks for the feedback. Did you define your children in paneProps as well?

const tabs = [
  {
    ...tabProps,
    pane: {
      children: (
        <div>
          <Message success icon='smile' content='Welcome to Tab #1!' />
          <p>
            This is the content of the main
          </p>
          <Form>
            <Field control='input' label='First Name' />
            <Field control='input' label='Last Name' />
            <Field.Checkbox label='I agree to the terms and conditions' />
            <Field.Button type='submit'>
              Sign Up
            </Field.Button>
          </Form>
        </div>
      ),
    }
  }
]

You could use an array there as well if you wanted children as siblings. I just used an expression.

@jeffcarbs
Copy link
Member

jeffcarbs commented Aug 24, 2016

We haven't gotten that far and currently it's somewhat coupled to our use-case. Basically the paneProps is just a function that renders html.


Edit: To clarify, our current implementation is actually more like:

<Menu>
  {tabs.map(tab, (props, index) => <Menu.Item {...props} active={index === this.state.selectedIndex} /> */}
</Menu>
{<Segment>{ tabs[this.state.selectedIndex].paneProps.renderer()}</Segment>  */}

I think this should definitely take advantage of the fact that the Tab component doesn't have custom styling. Like you said, it's just a Menu sitting on top of a Segment. The Segment gets the tab class but it looks like that's only to make it visible/hidden. If we're handling which pane is visible with react then that's unnecessary so you can just reuse existing components.

const tabs = [
  { 
    ...tabProps,
    pane: {
      ...paneProps
    }
  }
]

<Tab tabs={tabs} />

// Would render:

<Menu>
  {tabs.map(tab, (props, index) => <Menu.Item {...props} active={index === this.state.selectedIndex} /> */}
</Menu>
{<Segment {...tabs[this.state.selectedIndex].paneProps} />  */}

Personally we have a use-case for tab functionality but without the default attached/tabular style. I'd love to not have to maintain my own. I could see being able to pass menu props and segment props that would override the default (tabular/attached).

Or perhaps it would be better for the default to NOT have the tabular/attached styles applied and this component without customization would just provide tab functionality. It would make it easier to style custom and wouldn't really require much overhead for developers who want to use the tabular/attached menu styles.

<Tab tabs={tabs} attached='top' tabular />

// OR

<Tab tabs={tabs} menuProps={{attached:'top', tabular:true}} />

@brsanthu
Copy link
Contributor

Another good React tab implementation for reference http://zippyui.com/docs/react-tab-panel

@levithomason
Copy link
Member Author

Thanks for the link, I had checked out these and a few others, but not react-tab-panel:

https://github.com/reactjs/react-tabs
https://github.com/pedronauck/react-simpletabs

The common thread seems to be that ever child is a tab. It defines the tab name via a prop, and content via its children.

I like the idea of passing the array in though. Not sure where this will go yet.

@brsanthu
Copy link
Contributor

brsanthu commented Aug 25, 2016

I think passing the tab specific props to a tab like icon, label, if it is closeable, etc., However props that controls overall tabs should be able to specify at parent like if tabs should appear left or right or if any can be closed etc.,

An Example.

<Tabs tabPosition="left" labelAlignment="right">
  <Tab label='First' closeable>
    First
  </Tab>
  <Tab label='Second'>
    Second
  </Tab>
  <Tab label='Third'>
    Third
  </Tab>
</Tabs>

@levithomason
Copy link
Member Author

levithomason commented Aug 28, 2016

I've pleasantly realized that only active tab classes are needed to show the active tab. This means we can better separate these components. I think we'll support something like this:

<Tab as={Segment} />

This way the user can use any tab component. My use case simply required a div which would be the default tab. This then allows using any tab menu and any tab pane. I think then we would also do something like an activeIndex.

@levithomason levithomason changed the title Tab: add component feat(Tab): Add component Sep 20, 2016
@jeffcarbs
Copy link
Member

@levithomason - Would you mind if I took this PR off your hands? We're gonna need this soon. Based on recent work completed, I'm thinking:

<TabGroup>
  <Tab menuItem='Tab 1' as={Segment}>Tab 1 content</Tab>
  <Tab menuItem='Tab 2' as={Segment}>Tab 2 content</Tab>
  <Tab menuItem='Tab 3' as={Segment}>Tab 3 content</Tab>
<TabGroup>

// Would generate:
<Menu>
  <Menu.Item content='Tab 1' />
  <Menu.Item content='Tab 2' />
  <Menu.Item content='Tab 3' />
</Menu>
<Segment /* Tab 1 */>Tab 1 content</Segment>
<Segment /* Tab 2 */>Tab 2 content</Segment>
<Segment /* Tab 3 */>Tab 3 content</Segment>
  • TabGroup would be an autocontrolled component that could manage activeIndex or have it passed in.
  • menuItem would be a shorthand prop, so you could pass in the string label or an object to define the menu items.

One hangup, this is going to have to be wrapped in a div. I hate extra markup, but I don't see a way around that.

@levithomason
Copy link
Member Author

Sure thing, I'll provide review where I can. Head it up how it makes sense for you. My only guideline is that is support all valid SUI core usages and that the props parity the SUI names.

@jeffcarbs jeffcarbs force-pushed the feature/tabs branch 2 times, most recently from 7c9654c to fb25a5a Compare September 24, 2016 03:13
@jeffcarbs
Copy link
Member

jeffcarbs commented Sep 24, 2016

@levithomason - took a first pass at this, came out pretty well I think:

screen shot 2016-09-23 at 10 59 26 pm

Generated HTML:

<div>
  <div class="ui top attached tabular menu">
    <a class="item">Tab 1</a>
    <a class="active item">Tab 2</a>
    <a class="item">Tab 3</a>
  </div>
  <div class="ui bottom attached ui tab segment">Tab 1 Content</div>
  <div class="ui bottom attached ui active tab segment">Tab 2 Content</div>
  <div class="ui bottom attached ui tab segment">Tab 3 Content</div>
</div>

Some notes/thoughts:

  • Currently using the name Tab.Segment for each "tab", although it doesn't default to a Segment under the hood, so might be less confusing to call it Tab.Pane or Tab.Panel.
  • The menu prop on tab is a way to customize the menu, any props (e.g. className) passed directly to Tab will be spread on the wrapping div.
  • We're managing activeIndex in the Tab AND in the menu. Could we only do it in one spot? Ideally we could just do menuChild.state.activeIndex but I don't think that's how React works haha. Answered my own question as I was typing this, see fb25a5a :)

I'm using the Menu's new items shorthand prop, which made things so much easier but has some rough edges that I think we can smooth out (and might make sense to do on this PR):

  • Each "item" in the array needs to be an object. Ideally they could just be shorthand props. Fixed, see comment below
  • When the items shorthand prop is passed, the Menu infers activeIndex from the item with an 'active' prop in componentWillMount. We probably shouldn't do that and instead rely on the activeIndex because otherwise onMount you'd set activeIndex one way but when you're updating props you'd be setting it another. DropdownItems don't have an active state, it's determined from value - I think we should do the same here. Fixed, see comment below

@brsanthu
Copy link
Contributor

brsanthu commented Sep 24, 2016

Does it makes sense to name it <Tab.Group> to be consistent with other places like <Field.Group>?

@jcarbo thank you for taking this up.

@codecov-io
Copy link

codecov-io commented Sep 24, 2016

Codecov Report

Merging #430 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
+ Coverage   99.75%   99.75%   +<.01%     
==========================================
  Files         142      144       +2     
  Lines        2441     2493      +52     
==========================================
+ Hits         2435     2487      +52     
  Misses          6        6
Impacted Files Coverage Δ
src/modules/Tab/Tab.js 100% <100%> (ø)
src/modules/Tab/TabPane.js 100% <100%> (ø)
src/collections/Menu/Menu.js 100% <100%> (ø) ⬆️
src/modules/Search/Search.js 100% <0%> (ø) ⬆️
src/elements/Input/Input.js 100% <0%> (ø) ⬆️
src/modules/Checkbox/Checkbox.js 100% <0%> (ø) ⬆️
src/addons/TextArea/TextArea.js 100% <0%> (ø) ⬆️
src/modules/Dropdown/Dropdown.js 100% <0%> (ø) ⬆️
src/elements/Button/Button.js 100% <0%> (ø) ⬆️
src/collections/Form/Form.js 100% <0%> (ø) ⬆️
... and 2 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 f7a7052...f689f91. Read the comment docs.

@jeffcarbs
Copy link
Member

Re: Tab vs Tab.Group, I could see both ways

  • Tab is consistent with the naming convention in this project of the top-level component mapping 1-1 with the SUI component name (e.g. http://semantic-ui.com/modules/tab.html)
  • Tab.Group with nested Tabs is more intuitive when thinking/talking about it

I would 100% agree with Tab.Group if the SUI markup were:

<div class='ui tabs'>
  <div class='ui tab'></div>
  <div class='ui tab'></div>
  <div class='ui tab'></div>
</div>

but the Tab module is really just a loose coupling of a Menu + block-level components (e.g. a Segment). So I kinda think the naming consistency of Tab as the top-level component wins out, although I'll defer to @levithomason here.


I wound up addressing the points related to the Menu items shorthand. In doing so, I think I figured out a pretty elegant solution to the problem of generating keys for arrays of shorthand props. I updated the createShorthandfactory to take two special props as extraProps to generate a key if missing. The props are:

  • childKey - enables explicitly setting the key via props
  • getChildKey - function that takes the given props and returns a key.

The update to the factory is at ae70c05 and usage can be see at fdf38cd

@jeffcarbs
Copy link
Member

jeffcarbs commented Sep 24, 2016

Thinking about how to make this a one-liner configured by data...

// This
<Tab menu={{ attached: 'top', tabular: true }}>
  <Tab.Segment as={Segment} attached='bottom' menuItem='Tab 1'>Tab 1 Content</Tab.Segment>
  <Tab.Segment as={Segment} attached='bottom' menuItem='Tab 2'>Tab 2 Content</Tab.Segment>
  <Tab.Segment as={Segment} attached='bottom' menuItem='Tab 3'>Tab 3 Content</Tab.Segment>
</Tab>

// Could be written as:
const items = [
 menuItem: 'Tab 1', as: Segment, attached: 'bottom', content: 'Tab 1 Content',
 menuItem: 'Tab 2', as: Segment, attached: 'bottom', content: 'Tab 2 Content',
 menuItem: 'Tab 3', as: Segment, attached: 'bottom', content: 'Tab 3 Content',
]

<Tab menu={{ attached: 'top', tabular: true }} items={items} />

The shorthand factory makes this abstraction really easy. I could even see only supporting the latter (data-driven approach) since the former relies on dealing with children.

Edit In fact, thinking about it more, the shorthand factory takes elements so this would be equally valid:

const items = [
  <Tab.Segment as={Segment} attached='bottom' menuItem='Tab 1'>Tab 1 Content</Tab.Segment>,
  <Tab.Segment as={Segment} attached='bottom' menuItem='Tab 2'>Tab 2 Content</Tab.Segment>,
  <Tab.Segment as={Segment} attached='bottom' menuItem='Tab 3'>Tab 3 Content</Tab.Segment>,
]

<Tab menu={{ attached: 'top', tabular: true }} items={items} />

I think this may be the best approach.

Copy link
Member Author

@levithomason levithomason left a comment

Choose a reason for hiding this comment

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

Naming

Agreed. Let's keep top level API parity in tact. We also strive to keep the sub component API 1:1 with SUI component parts.

Since the only component part is a tab (.ui.tab > .tab.active) we would technically do Tab.Tab. IMO, this is very unintuitive and we should pick something else. I propose we go with Tab.Pane since there is technically no required .ui.tab > .segment component part either. It is just that SUI uses that as the default because it can be bottom attached to the tabular menu easily.

Docs

It seems there is an issue with the loading example, the loading Segment is always visible:

image

Data Driven & Variations

I really like the data driven approach. It makes the component logic so much cleaner. It also is inline with our shorthand props / sub component API pattern.

Per the tabular menu variations, there are some valid uses cases I can't see how to cover with the current API.

The 3rd tabular example includes a right sub menu. If we used menu shorthand I think we should also expose a Tab.Menu component. This would default to as: Menu. Then the user can define any JSX needed for more complicated menus. Not sure of a good way to organize this since there are no child elements in the SUI markup to work with. Perhaps this?:

<Tab>
  <Tab.Menu />
  <Tab.Pane />
  <Tab.Pane />
</Tab>

The 4th and 5th tabular menu examples show a Menu and Segment that exist in separate places, Grid Columns. I'm out of time to explore this more ATM, but one crazy idea comes to mind for allow this somehow:

const { TabMenu, TabPanes } = Tab.create(menu, tabs)

I'm not even sure how this would work, just tossing ideas out there for now.


import TabSegment from './TabSegment'

class Tab extends Component {
Copy link
Member Author

Choose a reason for hiding this comment

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

Let's add the SUI tab description as a doc block above the Tab here. This gives the page level decription in the docs.

@levithomason
Copy link
Member Author

levithomason commented Sep 25, 2016

Interesting high level perspective here, there is no SUI tab component actually. There is only css to show and hide the active tab. The menu and tab themselves have no visual styles. All the styling is handled by the component you use as a menu and as a tab (ie Menu, Segment).

Since React can show and hide content easily, we're not actually limited at all by the SUI tab classes nor markup. We could actually deviate 100% to make the most sane React tabs and not lose anything. The only thing to lose AFAICT is the visibility CSS associated with the ".tab.active" className, which we could care less about if we choose.

Food for thought.

@levithomason
Copy link
Member Author

@jcarbo did you end up with a final solution here in your own apps?

@jeffcarbs
Copy link
Member

We wound up with something similar to what was described above:

const items = [
 menuItem: 'Tab 1', content: 'Tab 1 Content',
 menuItem: 'Tab 2', content: 'Tab 2 Content',
 menuItem: 'Tab 3', content: 'Tab 3 Content',
]

<Tab menu={{ attached: 'top', tabular: true }} items={items} />

While there's no class for the tab group wrapper, there's a tab class that gets applied to the content (e.g. segment) which has two modifying classes:

  • active - determines visibility (like you mentioned, not super important given React)
  • loading - shows a spinner

So there's at least one visual style that this component would provide, along with the basic tab functionality.

I'll try to clean this PR up a little and see where we're at.

@UnbrandedTech
Copy link
Contributor

One thing I'd like to mention is "async tabs" where you can pass a callback to the tab panes to fetch it's children. In our application there's a page we need that has 4 tables tabbed out. Each table is large and we'd like to not render it unless that tab is active.

Just a thought.

@AlvMF1
Copy link

AlvMF1 commented Dec 20, 2016

Available before Xmas? =]

@levithomason
Copy link
Member Author

Cutting it close!

@mclarentgp
Copy link
Contributor

I don't mean to be annoying, but would just really like to be able to use the tab functionality and was wondering if there is a planned eta to make this available in the latest npm build?

@levithomason
Copy link
Member Author

No worries, I don't take checkups to be annoying :) The ETA is "as soon as possible", though, this module isn't required by any paid work so it is certainly deprioritized.

Completion wise, I believe I'm well over 90% here. Just needs to more tests and cleanup. Any help appreciated. You can PR against this branch to help out. Just run npm run lint to see what is failing the linter, and npm run test:watch to run, add, and fix tests. Once these things are passing, I believe we're good to go.

@dfsp-spirit
Copy link

This looks cool and almost ready. Is there any chance this will get the last few fixes so it can go into master?

@levithomason
Copy link
Member Author

There is indeed a really good chance. It has been on the bucket list for quite some time and has gotten to my nerves.

@jseminck
Copy link

jseminck commented May 25, 2017

If you need some help, I don't mind to take a look.

  • I see there are a handful of linting errors (that are trivial to fix).
  • There are a bunch of warnings. (but they are not related to the tabs module)
  • There are about 33 tests failing.

Edit: I took a look at this. After the last commit (39c928b) it seems like a lot has changed. I have seen that;

  • Docs are now out of date. I tried fixing them, but the result looked incorrect. There were UI issues with borderless, bottom position, and I believe tabular as well.
  • TabPane seems like it should be removed completely?
  • The tests need to be refactored as they still refer to the old implementation where TabPane should be rendered as children of Pane

It does seem like an interesting task but I am not sure as a n00b to the project it is a good idea. :)

@levithomason
Copy link
Member Author

@jseminck thanks for the deep dive into the state of things. I'm mid-way through changing the API. Perhaps after I solidify the component and the initial example there will be room for a handoff or pair effort here. Very much appreciated.

@levithomason levithomason force-pushed the feature/tabs branch 2 times, most recently from 74f9c7d to 79773d9 Compare June 1, 2017 18:20
@levithomason
Copy link
Member Author

@jeffcarbs @brsanthu @UnbrandedTech @AlvMF1 @dpkwhan @mclarentgp @dfsp-spirit @jseminck

This component is ready for some user testing and feedback.

How?

If you could pull this branch feature/tabs, npm install,npm start, and navigate to http://localhost:8080/modules/tab, you'll see the Tab component examples. Please have a look at the code, play with the editors, and let me know what you think.

FYI

I went with render() methods in the tab pane shorthand so as not to render every single tab ahead of time. This way, they are only rendered when the tab is made active.

I do think there is room for a single top level renderPane or defaultPaneProps prop to prevent repeating props for every tab pane in the list. You'll see what I mean in some of the examples.


Let me know your thoughts!

@layershifter
Copy link
Member

@levithomason Awesome work 👍 I've added initial typings and some style fixes.

However, I don't think that render() is the best idea, I don't remember places where we use something similar 🤔

@mclarentgp
Copy link
Contributor

mclarentgp commented Jun 5, 2017

@levithomason Looks great, awesome work!

I was curious though since all tab examples are horizontal, if it was possible to do a vertical set up with the tabs being on right or left side of the segment? I tried to get an example working vertically, but unfortunately wasn't able to do so.

@levithomason
Copy link
Member Author

render()

@layershifter My thought here was based on @UnbrandedTech's #430 (comment):

...Each table is large and we'd like to not render it unless that tab is active.

The render method then allows you define tab content that will not be rendered until that tab is made active. I think in this case it may be an acceptable departure from the pattern found in, say the Accordion, where all panes are rendered on initialization.

Let me know your thoughts.

Vertical

@mclarentgp I initially had a smarter Tab that managed the Menu and Segment with more features, but removed it in order to get this out the door. I would love to support vertical tabs immediately following this PR, hopefully a community contribution.

The thing with vertical tabs is that the Menu and Segment need to be separated by a Grid with stretched columns in order to work. See the tabular menu example in the SUI core docs: https://semantic-ui.com/collections/menu#tabular

@levithomason
Copy link
Member Author

Added remaining docs.
Updated Menu to work with activeIndex strings.
Starting updating tests.

Just need to finish Tab test updates here and we're good to go.

@mclarentgp
Copy link
Contributor

@levithomason
I've been playing around locally with trying to get veritcal tabs to work and so far this is what I've come up with. I dont have a lot of in depth experience with react so bear with me as it may not be the ideal or best implementation.

I did this in the tab.js file and imported the Grid and GridColumn controls into the file
static defaultProps = { menu: { attached: true, tabular: true, vertical: false, fluid: false }, grid: { segmentWidth: 12, tabWidth: 4 }, }

` renderColumn(columnProps) {
return GridColumn.create(columnProps)
}

renderVertical(menu) {
const { grid, panes } = this.props
const { activeIndex } = this.state

return (
   <Grid>
    {menu.props.tabular !== 'right' && this.renderColumn({ width: grid.tabWidth, children: menu })}
    {this.renderColumn({
      width: grid.segmentWidth,
      children: _.invoke('render', panes[activeIndex], this.props),
      stretched: true }
    )}
    {menu.props.tabular === 'right' && this.renderColumn({ width: grid.tabWidth, children: menu })}
  </Grid>
)

}

render() {
const { panes } = this.props
const { activeIndex } = this.state

const menu = this.renderMenu()
const grid = this.renderVertical(menu)
const rest = getUnhandledProps(Tab, this.props)
const ElementType = getElementType(Tab, this.props)

return (
  <ElementType {...rest}>
      {menu.props.vertical === true && grid}
      {menu.props.vertical !== true && menu.props.attached !== 'bottom' && menu}
      {menu.props.vertical !== true && _.invoke('render', panes[activeIndex], this.props)}
      {menu.props.vertical !== true && menu.props.attached === 'bottom' && menu}
  </ElementType>
)

}`

added the following to GridColumn.js
GridColumn.create = createShorthandFactory( GridColumn, stretched => ({ stretched }), width => ({ width }), children => ({ children }) )

and then modified one of the existing examples to test this out.
`import React from 'react'
import { Tab } from 'semantic-ui-react'

const panes = [
{ menuItem: 'Tab 1', render: () => <Tab.Pane>Tab 1 Content</Tab.Pane> },
{ menuItem: 'Tab 2', render: () => <Tab.Pane>Tab 2 Content</Tab.Pane> },
{ menuItem: 'Tab 3', render: () => <Tab.Pane>Tab 3 Content</Tab.Pane> },
]

const TabExampleAttachedBottom = () => (
<Tab menu={{ fluid: true, vertical: true, tabular: 'right' }} panes={panes} grid={{ segmentWidth: 8, tabWidth: 4 }} />
)

export default TabExampleAttachedBottom`

What are your thoughts? Is this even a valid/good way of achieving vertical tabs?

@levithomason
Copy link
Member Author

Indeed, this is the general direction I think it could go. I initially had a similar solution, importing the Grid and wrapping the Menu / Segment when there was a vertical Menu. I'd be up for iteration on something like this in a separate PR after this one is merged. Working on it now.

@levithomason
Copy link
Member Author

It is done!

@levithomason
Copy link
Member Author

Released in [email protected]

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

Successfully merging this pull request may close these issues.