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

[RFR] Add options prop to TabbedShowLayout #2740

Merged
merged 10 commits into from
Jan 15, 2019
Merged

[RFR] Add options prop to TabbedShowLayout #2740

merged 10 commits into from
Jan 15, 2019

Conversation

lucas2595
Copy link
Contributor

After facing the same problems as #2317, this PR allows to pass props through <TabbedShowLayout> to material-ui's <Tabs>. This way the user can enable all the different Tabs modes (such as scrollable, centered), and pass other props that might be useful. This is done through the options prop in <TabbedShowLayout>.

@lucas2595 lucas2595 changed the base branch from master to next January 8, 2019 19:02
@lucas2595 lucas2595 changed the base branch from next to master January 8, 2019 19:03
@fzaninotto
Copy link
Member

Thanks for your PR. I have a better idea to solve this problem: add a tabs prop that defaults to the <Tabs> component as it is today, to let developers override it. See e.g. the body prop of the <Datagrid> component as an example.

Instead of using an "options" prop to pass props to the <Tabs> in
<TabbedShowLayout>, now a whole <Tabs> custom component can be passed
through the new "tabs" prop.
@lucas2595
Copy link
Contributor Author

That indeed sounds better. Just made another commit to it implementing that.

@fzaninotto
Copy link
Member

Please make this PR against next instead of master as it's a new feature.

Instead of using an "options" prop to pass props to the <Tabs> in
<TabbedShowLayout>, now a whole <Tabs> custom component can be passed
through the new "tabs" prop.
@lucas2595 lucas2595 changed the base branch from master to next January 15, 2019 12:12
@lucas2595
Copy link
Contributor Author

Sorry about the messed up history, first time forking and making a PR on github here. All those force-pushes were to fix a rebase gone wrong. Should I redo it from scratch to clean things up?

@fzaninotto
Copy link
Member

I can squash and merge, don't worry about that.

Previously it was being passed to its children.
@fzaninotto fzaninotto merged commit ce9ddda into marmelab:next Jan 15, 2019
@fzaninotto
Copy link
Member

Terrific, thanks a lot!

@fzaninotto fzaninotto added this to the 2.7.0 milestone Jan 15, 2019
@MROFerreiro
Copy link

MROFerreiro commented Feb 13, 2019

Is this really working? I tried to use an options object with props defined in material-ui tabs component in <TabbedShowLayout> like: <TabbedShowLayout options={{scrollable=true}} /> but it doesn't work. How is this supposed to be used?
For what I saw in source code, there is no options props treated by <TabbedShowLayout> neither in <TabbedShowLayoutTabs> the new tabs. Am I missing something?
I also tried to import <TabbedShowLayoutTabs> and give it the scroll option, but is isn't exported at framework level.

@lucas2595
Copy link
Contributor Author

I'm sory @MROFerreiro, the PR title became misleading after it was decided to use a tabs prop in <TabbedShowLayout> like: <TabbedShowLayout tabs={<TabbedShowLayoutTabs/>}> (where <TabbedShowLayoutTabs/> is the old tabs component that was being used before this PR) instead of the options props suggestion.
For what you're trying to do, you can either use <TabbedShowLayout tabs={<TabbedShowLayoutTabs scrollable={true}/>}> (since the props passed to TabbedShowLayoutTabs are passed to the material-ui's Tabs inside it), or you can create your own tabs component, and then use it like: <TabbedShowLayout tabs={<MyCustomTabs/>}>.
@fzaninotto should I edit the PR's title to prevent this confusion?

@MROFerreiro
Copy link

Currently is not possible to use your solution with <TabbedShowLayoutTabs> since it is not export in index of details simply adding export {default as TabbedShowLayoutTabs} from './TabbedShowLayoutTabs' would fix it, currently I copied your implementation to a custom component and used it. Seems fine, but I need to do some corrections on my side in order for it to work properly.
That said, @fzaninotto, since is a simple hotfix I don't think is needed to do another merge request.

@fzaninotto
Copy link
Member

Please open a new PR to export and document TabbedShowLayoutTabs

@MROFerreiro
Copy link

@lucas2595 can you do it? Since you were the one that build this feature you would produce better docs than I and faster. Don't forget the export of the new Tabs component. 😃

@lucas2595
Copy link
Contributor Author

Sure thing, @MROFerreiro. Sorry for the delay, got caught up with with some other stuff in the past days. I'll do it right now.

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

Successfully merging this pull request may close these issues.

3 participants