breaking(Tab): change menu.aligned prop to menuPosition and infer menu.tabular from it #2499

amankkg · 2018-02-06T18:05:43Z

BREAKING CHANGE

The menu.aligned prop has been replaced with menuPosition. This keeps the menu prop strictly equal to the <Menu />'s props.

Before

const menu = {  fluid: true, vertical: true, aligned: 'right' }

<Tab panes={[...]} menu={menu} />

After

const menu = {  fluid: true,  vertical: true }

<Tab panes={[...]} menu={menu} menuPosition="right" />

The current implementation of Tab's menu.aligned prop triggers React's design-time warning on redundant aligned prop being passed down to div of underlying Menu.
In this change new dedicated menuPosition prop is introduced and old menu.aligned prop is removed.

Remaining behavior is not changed:

supported values is PropTypes.oneOf(['left', 'right'])
non-required and default value is 'left'

Resolves #2430

welcome · 2018-02-06T18:05:45Z

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

Run yarn lint locally to catch formatting errors. This will fix some errors automatically, commit and push any changes.
Run yarn test locally to catch errors. This ensures all components still behave as they should.
Run yarn start to run the doc site locally and try a few pages, ensuring everything is in good working order.
Include tests when adding/changing behavior.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

codecov-io · 2018-02-06T18:10:46Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2499      +/-   ##
==========================================
+ Coverage   99.74%   99.74%   +<.01%     
==========================================
  Files         160      160              
  Lines        2758     2762       +4     
==========================================
+ Hits         2751     2755       +4     
  Misses          7        7

Impacted Files	Coverage Δ
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 8114d8b...f7a66da. Read the comment docs.

amankkg · 2018-02-06T18:10:56Z

Any feedback on prop name? menuPosition is OK too, IMO, sounds similar to Input's label and labelPosition props.

levithomason · 2018-02-07T00:02:49Z

Ah, yes, menuPosition is more fitting.

amankkg · 2018-02-07T17:11:03Z

Done!

levithomason · 2018-02-07T18:12:33Z

Vertical Example

This example shows the tabs on the wrong side of the pane.

http://localhost:8080/modules/tab#tab-example-vertical-true

It seems we also have conflicting configuration now between the props menu.tabular and menuPosition. The menu.tabular prop should only take a boolean now that we have menuPosition. The direction of a tabular menu should be derived from the menuPosition.

levithomason · 2018-02-07T18:24:24Z

I see, we'll still need menuPosition for the cases where you need position a non-tabular menu, such as a secondary menu.

I think it would be ideal to infer the tabular value from the menuPosition, if provided. So, that menuPosition='right' would automatically use a tabular='right' menu. Otherwise, the user has to configure both to "right" to make it appear correctly.

amankkg · 2018-02-08T20:25:16Z

@levithomason

This example shows the tabs on the wrong side of the pane.

But why? If there is no menuPosition prop provided - it defaults to 'left'. And tabular="right" is only applied on Menu itself.

It seems we also have conflicting configuration now between the props menu.tabular and menuPosition. The menu.tabular prop should only take a boolean now that we have menuPosition. The direction of a tabular menu should be derived from the menuPosition.

I think it would be ideal to infer the tabular value from the menuPosition, if provided. So, that menuPosition='right' would automatically use a tabular='right' menu. Otherwise, the user has to configure both to "right" to make it appear correctly.

OK. We ~~can~~ should infer menu's tabular value from the menuPosition prop.
But, as user, I'd expect that Tab's menu API to align Menu's API. And Menu's API on tabular prop is expecting one of false | true | 'right'. So menu.tabular shouldn't accept boolean only.
And, if we're going to infer menu's tabular value, then Menu's tabular should accept 'left' also, this way we can specify Tab's menu.tabular as 'left' and menuPosition as 'right'.

amankkg · 2018-02-08T20:31:06Z

src/modules/Tab/Tab.js

    const { activeIndex } = this.state

+    if (menu.tabular === true) {
+      menu.tabular = menuPosition === 'left' || 'right'
+    }


menu.tabular = menuPosition === 'left' || 'right'
since there is no support for value 'left' in menu.tabular, we cannot set menu.tabular as 'left' AND menuPosition as 'right'

amankkg · 2018-02-08T20:38:52Z

There is the logic of inferring of menu.tabular at 4d6a9c4 (comments are visible here).
If it's OK we need to update Menu propTypes and handle its tabular's value 'left' to be similar as true.
And fix tests, docs, etc.

amankkg · 2018-02-08T20:43:40Z

src/modules/Tab/Tab.js

@@ -114,7 +114,7 @@ class Tab extends Component {
    const { activeIndex } = this.state

    if (menu.tabular === true) {
-      menu.tabular = menuPosition === 'left' || 'right'
+      menu.tabular = menuPosition
    }


menu.tabular = menuPosition
this way we can specify menu.tabular as true if we just want to infer it from menuPosition
and as left or right if we want to specify ourselves

amankkg · 2018-02-09T21:16:52Z

Now menu.tabular supports 'left' and it derives final value only if set to true. This way we can specify menu.tabular: 'left' and menuPosition: 'right' simultaneously (which will became tabular: true, not 'right').

amankkg · 2018-02-09T21:20:14Z

I'd like to see Menu's tabular accepting 'left' also, even if its behavior remains similar to false. If so, Tab's menu prop would match Menu's props.

amankkg · 2018-02-09T21:22:47Z

@levithomason I'll update the rest of the code if it's OK now.

levithomason · 2018-02-14T07:28:43Z

I'd like to see Menu's tabular accepting 'left' also, even if its behavior remains similar to false. If so, Tab's menu prop would match Menu's props.

While I agree, let's save this for another PR. There are many cases of props taking boolean values along with a "right" or "bottom" but not accepting a "left" or "top".

The only other comment I have here is that I think we need to simplify the prop computation. The prop description has too many considerations given the scope of the feature (positioning the menu). Ideally, the user wouldn't have to think or make decisions beyond a left/right decision for the position. Anything more than this I feel reduces the utility of the component.

I apologize I as I don't have time to offer suggestions at the moment. Feel free to offer your thoughts on this.

levithomason · 2018-02-18T21:12:23Z

@amankkg How's your availability looking for wrapping this one up?

amankkg · 2018-02-19T04:55:03Z

Oh, sorry for delay. I'll continue this asap 😎

…

On Mon, Feb 19, 2018, 3:12 AM Levi Thomason ***@***.***> wrote: @amankkg <https://github.com/amankkg> How's your availability looking for wrapping this one up? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2499 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADwDZEasq94XjLpAloylMig-U-TwwaBcks5tWJI6gaJpZM4R7fAq> .

amankkg · 2018-02-19T19:31:45Z

@levithomason

While I agree, let's save this for another PR.

Yep, sure.

amankkg · 2018-02-20T17:59:41Z

@levithomason there is an ugly prop computation because of Menu's aligned prop API is one of...

There are many cases of props taking boolean values along with a "right" or "bottom" but not accepting a "left" or "top"

Not sure if this can simplified without decreasing Menu's customizeability.
Otherwise, there is no way to specify menuPosition to be right and menu.tabular to be left at the same time, as example.

Any comments and proposals are welcome, especially on props description in documentation.

upd: example case

amankkg · 2018-03-10T20:38:13Z

Updated test cases and added new example for menu.tabular prop in docs.

levithomason · 2018-03-18T16:14:14Z

This looks great! I will do some cleanup and merge this today.

levithomason · 2018-03-18T17:39:31Z

Added/updated docs, updated logic a bit and tests. Now menuPosition always wins, but, menu.tabular can also be used to set the position, just as with a <Menu />.

amankkg · 2018-04-02T06:32:27Z

@levithomason what is the status of this PR? What can I do to help?

levithomason · 2018-04-13T15:56:17Z

Sorry for the delay. Merging master and running tests and we'll ship this!

levithomason · 2018-04-30T19:19:11Z

Rebased and updated. Really hoping to merge this now...

…React into fix/tab-menu-warning-redundant-prop-aligned

layershifter · 2018-05-01T12:17:45Z

Removed cruft example, tests passed 👍

welcome · 2018-05-01T12:17:52Z

Congrats on merging your first pull request! 🎉🎉🎉

levithomason · 2018-05-02T06:17:45Z

Released in semantic-ui-react@0.80.0.

levithomason added the needs author feedback label Feb 7, 2018

amankkg changed the title ~~breaking(Tab): change menu.aligned prop to menuAligned~~ breaking(Tab): change menu.aligned prop to menuPosition Feb 7, 2018

amankkg commented Feb 8, 2018

View reviewed changes

amankkg mentioned this pull request Feb 8, 2018

Tab component passing redundant aligned prop to underlying Menu > div #2430

Closed

amankkg changed the title ~~breaking(Tab): change menu.aligned prop to menuPosition~~ breaking(Tab): change menu.aligned prop to menuPosition and infer menu.tabular from it Feb 9, 2018

levithomason mentioned this pull request Feb 16, 2018

Fixed the redundant aligned prop passing to div issue #2530

Closed

levithomason added ready for review and removed needs author feedback labels Feb 26, 2018

levithomason added the ready for merge label Mar 18, 2018

levithomason force-pushed the fix/tab-menu-warning-redundant-prop-aligned branch from 9ad74c0 to c76ed41 Compare March 18, 2018 17:38

breaking(Tab): prop menu.aligned to menuPosition

148f973

levithomason force-pushed the fix/tab-menu-warning-redundant-prop-aligned branch from 98031df to 148f973 Compare April 30, 2018 19:18

levithomason removed the ready for review label Apr 30, 2018

layershifter added 2 commits May 1, 2018 14:32

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

0326231

…React into fix/tab-menu-warning-redundant-prop-aligned

docs(Tab): remove cruft TabExampleTabularVerticalRight example

f7a66da

layershifter merged commit e3ac2bd into Semantic-Org:master May 1, 2018

amankkg deleted the fix/tab-menu-warning-redundant-prop-aligned branch May 2, 2018 06:56

levithomason added the breaking-change label Dec 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

breaking(Tab): change menu.aligned prop to menuPosition and infer menu.tabular from it #2499

breaking(Tab): change menu.aligned prop to menuPosition and infer menu.tabular from it #2499

amankkg commented Feb 6, 2018 •

edited by levithomason

Loading

welcome bot commented Feb 6, 2018

codecov-io commented Feb 6, 2018 •

edited

Loading

amankkg commented Feb 6, 2018

levithomason commented Feb 7, 2018

amankkg commented Feb 7, 2018

levithomason commented Feb 7, 2018

levithomason commented Feb 7, 2018

amankkg commented Feb 8, 2018

amankkg Feb 8, 2018 •

edited

Loading

amankkg commented Feb 8, 2018 •

edited

Loading

amankkg Feb 8, 2018 •

edited

Loading

amankkg commented Feb 9, 2018 •

edited

Loading

amankkg commented Feb 9, 2018 •

edited

Loading

amankkg commented Feb 9, 2018 •

edited

Loading

levithomason commented Feb 14, 2018

levithomason commented Feb 18, 2018

amankkg commented Feb 19, 2018 via email

amankkg commented Feb 19, 2018

amankkg commented Feb 20, 2018 •

edited

Loading

amankkg commented Mar 10, 2018

levithomason commented Mar 18, 2018 •

edited

Loading

levithomason commented Mar 18, 2018

amankkg commented Apr 2, 2018

levithomason commented Apr 13, 2018

levithomason commented Apr 30, 2018

layershifter commented May 1, 2018

welcome bot commented May 1, 2018

levithomason commented May 2, 2018

breaking(Tab): change menu.aligned prop to menuPosition and infer menu.tabular from it #2499

breaking(Tab): change menu.aligned prop to menuPosition and infer menu.tabular from it #2499

Conversation

amankkg commented Feb 6, 2018 • edited by levithomason Loading

BREAKING CHANGE

Before

After

welcome bot commented Feb 6, 2018

codecov-io commented Feb 6, 2018 • edited Loading

Codecov Report

amankkg commented Feb 6, 2018

levithomason commented Feb 7, 2018

amankkg commented Feb 7, 2018

levithomason commented Feb 7, 2018

Vertical Example

levithomason commented Feb 7, 2018

amankkg commented Feb 8, 2018

amankkg Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

amankkg commented Feb 8, 2018 • edited Loading

amankkg Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

amankkg commented Feb 9, 2018 • edited Loading

amankkg commented Feb 9, 2018 • edited Loading

amankkg commented Feb 9, 2018 • edited Loading

levithomason commented Feb 14, 2018

levithomason commented Feb 18, 2018

amankkg commented Feb 19, 2018 via email

amankkg commented Feb 19, 2018

amankkg commented Feb 20, 2018 • edited Loading

amankkg commented Mar 10, 2018

levithomason commented Mar 18, 2018 • edited Loading

levithomason commented Mar 18, 2018

amankkg commented Apr 2, 2018

levithomason commented Apr 13, 2018

levithomason commented Apr 30, 2018

layershifter commented May 1, 2018

welcome bot commented May 1, 2018

levithomason commented May 2, 2018

amankkg commented Feb 6, 2018 •

edited by levithomason

Loading

codecov-io commented Feb 6, 2018 •

edited

Loading

amankkg Feb 8, 2018 •

edited

Loading

amankkg commented Feb 8, 2018 •

edited

Loading

amankkg Feb 8, 2018 •

edited

Loading

amankkg commented Feb 9, 2018 •

edited

Loading

amankkg commented Feb 9, 2018 •

edited

Loading

amankkg commented Feb 9, 2018 •

edited

Loading

amankkg commented Feb 20, 2018 •

edited

Loading

levithomason commented Mar 18, 2018 •

edited

Loading