feat(Tab): Add vertical tabbing #1894

mclarentgp · 2017-07-23T22:32:38Z

In the current implementation it's impossible to set tabs to be in a vertical format.
This pr changes this.

codecov-io · 2017-07-23T23:06:51Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #1894      +/-   ##
==========================================
+ Coverage   99.75%   99.75%   +<.01%     
==========================================
  Files         145      145              
  Lines        2477     2484       +7     
==========================================
+ Hits         2471     2478       +7     
  Misses          6        6

Impacted Files	Coverage Δ
src/collections/Grid/GridColumn.js	`100% <100%> (ø)`	⬆️
src/modules/Tab/Tab.js	`100% <100%> (ø)`	⬆️

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 02daf0f...fcc71d3. Read the comment docs.

layershifter

@mclarentgp Thanks for PR, I've left some comments, but I need time to pay more attention to this PR.

layershifter · 2017-07-24T17:26:29Z

src/collections/Menu/Menu.d.ts

@@ -81,7 +81,7 @@ export interface MenuProps {
  stackable?: boolean;

  /** A menu can be formatted to show tabs of information. */
-  tabular?: boolean | 'right';
+  tabular?: boolean | 'right' | 'left';


Can you clarify necessity of left value? There is no special CSS for it, so it's left by default.

Ahh, sorry you are correct this doesn't need to be changed. Will undo changes.

layershifter · 2017-07-24T17:28:53Z

src/collections/Grid/GridColumn.js

@@ -112,4 +113,11 @@ GridColumn.propTypes = {
  width: PropTypes.oneOf(SUI.WIDTHS),
 }

+GridColumn.create = createShorthandFactory(


createShorthandFactory accepts only two agruments, in this case correct usage is:

createShorthandFactory(GridColumn, children => ({ children }))

I will update this to the correct usage.

mclarentgp · 2017-07-24T18:16:52Z

@DENSENDEW Sorry I don't quite understand? I'm still fairly new to some of this and it's my first PR.

levithomason · 2017-07-28T16:43:25Z

src/modules/Tab/Tab.js

@@ -96,19 +102,42 @@ class Tab extends Component {
    })
  }

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


Since this is a pass-through method, let's just call GridColumn.create instead of this.renderColumn and drop this method.

levithomason · 2017-07-28T16:44:48Z

src/modules/Tab/Tab.js

@@ -64,7 +69,8 @@ class Tab extends Component {
  ]

  static defaultProps = {
-    menu: { attached: true, tabular: true },
+    menu: { attached: true, tabular: true, vertical: false, fluid: false },
+    grid: { segmentWidth: 12, tabWidth: 4 },


In keeping with the Tab terminology, let's go with paneWidth instead of segmentWidth.

levithomason · 2017-07-28T16:53:58Z

src/modules/Tab/Tab.js

+        {menu.props.tabular !== 'right' && this.renderColumn({ width: grid.tabWidth, children: menu })}
+        {this.renderColumn({
+          width: grid.segmentWidth,
+          children: _.invoke(_.get(panes, `[${activeIndex}]`), 'render', this.props),


This line:

_.invoke(_.get(panes, `[${activeIndex}]`), 'render', this.props)

is duplicated in the main render() method. Let's pull this line into a method like renderActivePane() to contain it.

levithomason · 2017-07-28T16:55:10Z

src/modules/Tab/Tab.js

+        {this.renderColumn({
+          width: grid.segmentWidth,
+          children: _.invoke(_.get(panes, `[${activeIndex}]`), 'render', this.props),
+          stretched: true }


Closing curly on the same line as the closing paren and wrapping curly please.

- stretched: true } -}) + stretched: true, +})}

levithomason · 2017-07-28T17:01:53Z

src/modules/Tab/Tab.js

@@ -64,7 +69,8 @@ class Tab extends Component {
  ]

  static defaultProps = {
-    menu: { attached: true, tabular: true },
+    menu: { attached: true, tabular: true, vertical: false, fluid: false },


No need to add vertical: false, fluid: false as these are not set by default.

levithomason · 2017-07-28T17:07:14Z

src/modules/Tab/Tab.js

@@ -37,6 +39,9 @@ class Tab extends Component {
    /** Shorthand props for the Menu. */
    menu: PropTypes.object,

+    /** Shorthand props for the Grid. */
+    grid: PropTypes.object,


Shorthand can be a string, number, props object, or an element. It is then passed to a shorthand factory, such as Grid.create(grid).

In this case, the user cannot pass normal Grid props such as celled or padded because this object is not used in a shorthand factory for the Grid. This is some kind of configuration rather than shorthand.

I'd like to reserve the grid prop for actual Grid props so users can control that if needed. That said, I'm not so sure of the best API for these values. Here are some ideas, would love feedback:

Top level props

Make paneWidth and tabWidth top level props rather than nested in grid. This makes room for grid to be true shorthand, however, those props are only applicable if menu.vertical is set so it might be a bit odd as well.

Omit and use rest for shorthand

The current grid object needs to be spread on the Grid in order to be true Grid props. We could leave it as is and just omit paneWidth and tabWidth from the grid value before spreading it:

const { paneWidth, tabWidth, ...gridProps } = grid <Grid {...gridProps> {GridColumn.create({ width: tabWidth })} {GridColumn.create({ width: paneWidth })} </Grid>

I think I'm leaning toward the omit/rest approach, what are your ideas?

I think I like the idea of omit/rest as it appears to give the best solution to maintaining the true grid props and the pane/tab width props

@levithomason I went ahead and made the omit/rest change. Let me know if you decide you want to use a different approach.

levithomason · 2017-07-28T17:11:26Z

src/modules/Tab/Tab.js

-        {menu.props.attached !== 'bottom' && menu}
-        {_.invoke(_.get(panes, `[${activeIndex}]`), 'render', this.props)}
-        {menu.props.attached === 'bottom' && menu}
+        {menu.props.vertical === true && grid}


The vertical menu prop makes the case for two clean return values here. We can also avoid creating a null grid in the case of non-vertical menus:

if (menu.props.vertical) { return <ElementType {...rest}>{this.renderVertical(menu)}<ElementType> } // ...the old return value as-is

issues adderssed

levithomason · 2017-08-13T17:40:22Z

I've pushed a minor update and will merge on pass.

levithomason · 2017-08-18T03:59:55Z

Whoops, sorry for the delay!

levithomason · 2017-08-20T20:58:22Z

Released in semantic-ui-react@0.71.5

mclarentgp added 2 commits July 23, 2017 17:30

feat(Tab): Add vertical tabbing

a316777

fix failing tests

63ecb51

add vertical tab tests and allow menu prop tabular to accept left

1f29cb3

layershifter previously requested changes Jul 24, 2017

View reviewed changes

layershifter added the needs author feedback label Jul 24, 2017

mclarentgp added 2 commits July 24, 2017 15:04

fix tabular menu props and grid column shorthand factory

a33eb59

fix GridColumn shorthand single line

307d656

levithomason reviewed Jul 28, 2017

View reviewed changes

mclarentgp added 2 commits August 1, 2017 13:02

refactor veritcal tab changes per review

de98325

fix failing test

878da51

levithomason added ready for review and removed needs author feedback labels Aug 10, 2017

refactor(Tab): remove unnecessary render logic

fcc71d3

levithomason merged commit 86bd8b5 into Semantic-Org:master Aug 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Tab): Add vertical tabbing #1894

feat(Tab): Add vertical tabbing #1894

mclarentgp commented Jul 23, 2017

codecov-io commented Jul 23, 2017 •

edited

Loading

layershifter left a comment

layershifter Jul 24, 2017

mclarentgp Jul 24, 2017

layershifter Jul 24, 2017

mclarentgp Jul 24, 2017

mclarentgp commented Jul 24, 2017

levithomason Jul 28, 2017 •

edited

Loading

levithomason Jul 28, 2017

levithomason Jul 28, 2017

levithomason Jul 28, 2017

levithomason Jul 28, 2017

levithomason Jul 28, 2017 •

edited

Loading

mclarentgp Jul 31, 2017

mclarentgp Aug 1, 2017

levithomason Jul 28, 2017

levithomason commented Aug 13, 2017

levithomason commented Aug 18, 2017

levithomason commented Aug 20, 2017

feat(Tab): Add vertical tabbing #1894

feat(Tab): Add vertical tabbing #1894

Conversation

mclarentgp commented Jul 23, 2017

codecov-io commented Jul 23, 2017 • edited Loading

Codecov Report

layershifter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mclarentgp commented Jul 24, 2017

levithomason Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Top level props

Omit and use rest for shorthand

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason commented Aug 13, 2017

levithomason commented Aug 18, 2017

levithomason commented Aug 20, 2017

codecov-io commented Jul 23, 2017 •

edited

Loading

levithomason Jul 28, 2017 •

edited

Loading

levithomason Jul 28, 2017 •

edited

Loading