Skip to content

Tab using renderActiveOnly: false and the pane: <Tab.Pane> syntax chokes on SFCs #2176

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

Closed
BillyWM opened this issue Oct 9, 2017 · 6 comments
Labels

Comments

@BillyWM
Copy link

BillyWM commented Oct 9, 2017

Tab can't understand SFCs (Stateless Functional Components) that return a <Tab.Pane> element.
However, if you use the render: syntax you can return an SFC in the render function just fine.

Steps

  1. Insert a Tab component
  2. Set its renderActiveOnly property to false
  3. Provide the panes[] array as demonstrated in the docs
  4. For the pane property of each object in the panes[] array, provide a SFC instead of a <Tab.Pane>

Expected Result

The panes display as normal

Actual Result

The panes are stacked on top of each other at all times

Version

0.74.2

Testcase

https://codepen.io/billywm/pen/NaYvmx

Additonal info

Another testcase demonstrating that this works just fine with the render: syntax

https://codepen.io/billywm/pen/WZzZNx

(Unfortunately I can't use that for my current purposes because I can't have the content unmounting, so this is a breaking issue)

@BillyWM BillyWM changed the title Tab using renderActiveOnly: false and the Pane: <Tab.Pane> syntax chokes on SFCs Tab using renderActiveOnly: false and the {pane: <Tab.Pane>} syntax chokes on SFCs Oct 9, 2017
@BillyWM BillyWM changed the title Tab using renderActiveOnly: false and the {pane: <Tab.Pane>} syntax chokes on SFCs Tab using renderActiveOnly: false and the pane: <Tab.Pane> syntax chokes on SFCs Oct 9, 2017
@layershifter
Copy link
Member

Take a look on this example. It will show how shorthands works. The correct usage for your case will be:

{ pane: { content: <CustomPaneTwo /> }}

In the your codepen CustomPaneTwo replaces Tab.Pane. The discussion of this behaviour was there. BTW, we should add docs for shorthands, #561.

@BillyWM
Copy link
Author

BillyWM commented Oct 9, 2017

How is anyone ever supposed to deduce that from the docs? After all, the library can consume a Tab.Pane with the two other methods, and, as demonstrated, can consume a wrapper component with method #1.

There is no notation in your docs saying "syntax #2 can't understand wrapper components for some reason but this other one at the bottom of the page can".

The docs only say

You can use an (sic) item shorthands when you're using renderActiveOnly={false}.

"can" means that you have the option to, not that it's mandatory lest the library choke.

Sorry, but that is just flat-out bad API design. The expectation with using a React library is that one can embed components inside each other and the library just handles inserting the children. This is, in fact, a key to writing clean, well-engineered applications - modularization and separation of concerns. Having to feed your components into a particular property of a particular object in a certain way depending on exact circumstances, and having no documentation making it clear what is required and when, makes an API an absolute minefield.

This seems to me to be an artifact of the library internals. I expect that you've used TS static types and factory methods too strictly and the library can't properly consume generic components. An API this fiddly is a code smell.

(Lastly, the comment you linked to doesn't seem to have anything to do with Tab Panes)

@BillyWM
Copy link
Author

BillyWM commented Oct 9, 2017

For example, I can do this:

<Menu>
    <Menu.Item>
        <WhateverIWant />
    </Menu.Item>
</Menu>

and this:

<Grid>
    <Grid.Row>
        <Grid.Column>
            <WhateverIWant />
        </Grid.Column>
    </Grid.Row>
</Grid>

But when it comes to <Tab>......... I have to pass a <Tab.Pane> to a certain property on a Javascript object but if I'm using a wrapper object I have to pass it to the object in a different way but only if I've set a certain flag otherwise I can pass it in the first way again just fine.

Certainly it must be clear how counter-intuitive this is.

My expectation is I can just do this:

<Tab>
    <Tab.Pane>
        <WhateverIWant />
    </Tab.Pane>
<Tab>

The whole weird object API seems clunky and out of place to begin with.

@levithomason
Copy link
Member

levithomason commented Oct 16, 2017

The comparisons are not equal in this case. The Menu and Grid examples given are static and always render all of their children. A Tab is stateful only renders one child at a time. It doesn't make sense to render all the Tab.Panes of a Tab like you would all the Menu.Items of a Menu.
Also, the Tab example provided doesn't include the Menu, it only shows a single Tab. A Tab should always render the Menu, with stateful Menu.Items and only one Tab.

Minimal markup would actually look like this (note the active Menu.Item and Tab.Pane):

<Tab>
  <Menu>
    <Menu.Item active>Whatever I want</Menu.Item>
  <Menu>
  <Tab.Pane active>
    <WhateverIWant />
  </Tab.Pane>
<Tab>

This amount of markup is tedious and requires the developer to manage state. If you were going to write this amount of markup and state management, there is no use for the Tab as you could just replace <Tab> with <div> and <Tab.Pane> with <Segment> or what have you. You would have just rewritten the Tab component.

It is not idiomatic React to reach down the tree and parse children. It also fails when expected child components are wrapped in HOCs. Therefore, components that require stateful children must pass their children as props config.

Going back to the Menu example, if you want the Menu to manage the active Menu.Item, then you must pass the items as props as well. If you use the sub component API, then you must manage the state yourself. This is true of all SUIR components with stateful children.

@BillyWM
Copy link
Author

BillyWM commented Oct 16, 2017

It doesn't make sense to render all the Tab.Panes of a Tab like you would all the Menu.Items of a Menu.

Yet you've added such a feature to your project, the flag we're talking about? (renderActiveOnly=false)

If that's not the point of it then what is?

This amount of markup is tedious and requires the developer to manage state

Who are you quoting now? (Is this your words you accidentally quoted?)

@levithomason
Copy link
Member

Yet you've added such a feature to your project, the flag we're talking about? (renderActiveOnly=false)
If that's not the point of it then what is?

Note that this is opt-in. It doesn't make sense to force all users to render all tabs every time. In some cases, you may need to. This prop allows you to do that.

The quote is meant as a note to the code snippet.

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

No branches or pull requests

3 participants