Conversation
HammadTheOne
requested review from
Marc-Andre-Rivet and
alexcjohnson
as code owners
There was a problem hiding this comment.
@HammadTheOne This is looking good. Lots of coding suggestions and suggestions for updating the next_tab props based on previous discussion during demo.
Also, since it's the exact opposite functionality, I think we should also add Previous as part of this PR.
| className={tabClassName} | ||
| id="next-tab" | ||
| style={tabStyle} | ||
| onClick={() => { |
There was a problem hiding this comment.
Use the useCallback hook instead so as to not create a new function for this callback each time the component is rendered.
| }, | ||
| }; | ||
|
|
||
| const NextTab = ({ |
There was a problem hiding this comment.
Consider wrapping in React.memo to make this component Pure.
https://reactjs.org/docs/react-api.html#reactmemo
| }, | ||
| }; | ||
|
|
||
| const NextTab = ({ |
There was a problem hiding this comment.
This file is becoming very big. Consider breaking up the Tabs component file into multiple files. One way to do this would be to rename src/components/Tabs.react.js to src/components/Tabs.react/index.js (existing imports will recognize <folder>/index.js as the same as <folder>.js and won't require changes) and moving EnhancedTabs and NextTab into files of their own, referenced here.
| }); | ||
| } | ||
|
|
||
| if (this.props.children) { |
There was a problem hiding this comment.
| @@ -168,6 +258,7 @@ export default class Tabs extends Component { | |||
| render() { | |||
| let EnhancedTabs; | |||
There was a problem hiding this comment.
Not a change from this PR, but consider renaming to enhancedTabs - Pascal-Casing for variables deviates from the norm.
| next_tab: false, | ||
| persisted_props: ['value'], | ||
| persistence_type: 'local', | ||
| next_tab_layout: {title: 'Next', style: null}, |
There was a problem hiding this comment.
next_tab and next_tab_layout could be combined into a single navigation prop that would be extensible in the future. For example:
navigation: PropTypes.object({
next: PropTypes.oneOf([
PropTypes.bool,
PropTypes.object({
title: PropTypes.string,
style: PropTypes.object
})
])
})
Would default to false as implemented.
Using true would default to
{ title: 'Next', style: undefined }
Defining title, style would combine/override the defaults above.
There was a problem hiding this comment.
Later we can add Previous, Last, First, Hide Tabs, etc. to suit our needs.
There was a problem hiding this comment.
Instead of title, let's do label so that it matches dcc.Tab (
dash-core-components/src/components/Tab.react.js
Lines 18 to 21 in 3bcc238
| } | ||
|
|
||
| if (this.props.children) { | ||
| nextTab = () => { |
There was a problem hiding this comment.
This doesn't need to be a function, just create the instance of NextTab here directly
|
|
||
| dash_dcc.wait_for_element('#next-tab').click() | ||
|
|
||
| dash_dcc.wait_for_text_to_equal("#tabs-output", "tab-2") |
There was a problem hiding this comment.
Would like to see another test case without child tabs that makes sure Next is not present.
| (loading_state && loading_state.is_loading) || undefined | ||
| } | ||
| className={tabClassName} | ||
| id="next-tab" |
There was a problem hiding this comment.
This component fragment can't have a constant id. If there are multiple dcc.Tabs in the page, they will clash. You could use the component's id and append something to it like ${id}-next-tab.
There was a problem hiding this comment.
For example, EnhancedTab uses the child component's id, which should not be duplicated.
https://github.com/plotly/dash-core-components/pull/786/files#diff-87a94af645034b5ac219cbbfb96a6295R301
| }} | ||
| > | ||
| <span>{title}</span> | ||
| <style jsx>{` |
There was a problem hiding this comment.
🌴🐫 Share common styling for EnhancedTab / NextTab
https://github.com/plotly/dash-core-components/pull/786/files#diff-87a94af645034b5ac219cbbfb96a6295R64
3 participants
This PR addresses #685 and adds a next tab button to sequentially scroll through tabs within
dcc.Tabs. In it's current implementation, thenext_tabprop ofdcc.Tabsis a bool which determines whether or not it is added to the component.To Do:
Added a
next_tab_layoutprop. This is a object/dict which (hopefully) mimics the Plotly graphs styling, wheretitlemodifies the text of the button, andstyleis a object/dict which overrides the default styling of the tab and allows thenext-tabbutton to have separate styling from the rest of the component.Closes #685