fix: tabs component refactor #414

chrismanciero · 2019-02-15T23:35:31Z

Description

This is a complete refactor of the Tabs component which replaces the single file that contained both the TabGroup and Tab components with separate files for each component. This PR also adds a new component, TabContent, which is used to display the content of a tab. Each component resides in the /Tabs directory

/Tabs/Tab.js - a component used to display the clickable link of the tab strip
/Tabs/TabContent.js - a component used to display the contents of a selected tab
/Tabs/TabGroup.js - a component that wraps the Tab and TabContent components. The TabGroup handles the placement of the Tab strip as well as the content.

As seen on the Fiori Fundamentals Tab page (https://sap.github.io/fundamental/components/tabs.html). The Tabs with list element is the implementation used. The Tabs with nav element is not implemented as there does not seem to be a valid use-case to have 2 implementations.

fixes #344

bcullman · 2019-02-16T01:39:15Z

src/Tabs/TabContent.js

@@ -0,0 +1,36 @@
+import classnames from 'classnames';


@chrismanciero - the SAP Concur team discussed the pattern to be used for subcomponents, and wanted to use a "private" component structure.

to do this we typically prefix private components with underscores, and import/export them as part of the parent component. Reach out to @greg-a-smith or others for more guidance.

@chrismanciero Feel free to connect with me on Monday and I can explain more.

greg-a-smith · 2019-02-18T04:29:36Z

src/Tabs/Tab.js

+    return (
+        <a
+            {...rest}
+            {...tabLinkProps}


Now that these are all separate components, I don't think tabLinkProps is needed anymore. Any "other" props are already going to be included in the ...rest spread.

greg-a-smith · 2019-02-18T04:31:53Z

src/Tabs/Tab.js

+            className={linkClasses}
+            href={!disabled ? `#${id}` : null}
+            onClick={!disabled ? (event) => {
+                props.onClick(event, id);


onClick isn't defined as a prop and should be. It should also have an empty function as a default value. More on this below in the PR review...

greg-a-smith · 2019-02-18T04:32:43Z

src/Tabs/TabContent.js

@@ -0,0 +1,36 @@
+import classnames from 'classnames';


@chrismanciero Feel free to connect with me on Monday and I can explain more.

greg-a-smith · 2019-02-18T04:34:23Z

src/Tabs/TabContent.js

+
+    return (
+        <div
+            {...tabContentProps}


Not a huge deal, but with this being a separate component, the final spread (...tabContentProps) should probably be named something like ...rest or ...otherProps so it doesn't appear like it's a named prop.

greg-a-smith · 2019-02-18T04:36:15Z

src/Tabs/TabGroup.js

+                    className='fd-tabs__item'
+                    key={child.props.id}>
+                    <Tab {...child.props} onClick={this.handleTabSelection}
+                        selected={this.state.selectedId === child.props.id} />


The children of TabGroup should already be Tab components so you should be able to use React.cloneElement to pass additional/internal props to it.

greg-a-smith · 2019-02-18T04:53:08Z

src/Tabs/TabGroup.js

+TabGroup.propTypes = {
+    children: PropTypes.node,
+    className: PropTypes.string,
+    selectedId: PropTypes.string,


It's good there is a selectedId prop so the consumer can decide which tab is currently selected, however, there should also be a getDerivedStateFromProps (formerly componentWillReceiveProps) method to properly update state if the consumer changes that prop.

Additionally, there should be a callback function prop (named something like onTabChange) that the consumer can get updates in case they are managing any state related to these tabs.

greg-a-smith · 2019-02-18T04:58:27Z

src/Tabs/Tab.js

+            } : null}
+            role='tab'>
+            {title}
+        </a>


According to the WAI specs for Tabs, the element with role='tab' should actually be a <button>. Right out of the box, we will get better keyboard navigation support and have less to code ourselves.

That said, this may actually need to be an issue brought to the Fiori Fundamentals team since all libraries should adopt this change.

Yes, this needs to be brought up to Fiori Fundamentals because of styling issues. If i change from <a> tags to <button> tags the design looks bad

@greg-a-smith This change is part of the a11y story we have in the backlog for Tabs.

Awesome! 👍

greg-a-smith · 2019-02-18T05:20:28Z

src/Tabs/TabGroup.js

+                    {...this.props.tabContentProps}
+                    selected={this.state.selectedId === child.props.id}>
+                    {child.props.children}
+                </TabContent>);


This is an interesting approach (using an internal component) to help encapsulate the details for tab content. Per @bcullman's comment, we should just take a couple other steps to make sure this stays "private".

greg-a-smith · 2019-02-18T05:20:56Z

src/index.js

 export { SearchInput } from './SearchInput/SearchInput';
 export { SideNav, SideNavList, SideNavListItem } from './SideNavigation/SideNavigation';
+export { Tab } from './Tabs/Tab';
+export { TabContent } from './Tabs/TabContent';


I think the TabContent component should be internal-only and should not be exported.

greg-a-smith · 2019-02-18T05:21:58Z

src/Tabs/Tabs.test.js

@@ -1,101 +1,56 @@
 import { mount } from 'enzyme';


Now that these are separate components, we should have unit tests for each individually.

greg-a-smith · 2019-02-19T15:06:46Z

src/Tabs/Tab.js

+import React from 'react';
+
+export const Tab = (props) => {
+    const { title, className, disabled, glyph, id, selected, ...rest } = props;


Should probably add onClick to the list of props being pruned out.

greg-a-smith · 2019-02-19T15:28:30Z

src/Tabs/TabGroup.js

+    className: PropTypes.string,
+    selectedId: PropTypes.string,
+    tabContentProps: PropTypes.object,
+    tabLinkProps: PropTypes.object,


The props tabContentProps and tabLinkProps should be moved to the Tab component. Otherwise, a consumer will not be able to provide different props for each tab and its content. They can still be referenced in the render methods in this file, but would just come from child.props within those loops.

Taking that a step further, once moved to the Tab component, I think tabLinkProps could just become linkProps and those would be spread to the <a> element. Additionally, the Tab component could render its own <li> and spread ...rest to that element.

greg-a-smith · 2019-02-19T15:34:13Z

src/Tabs/TabGroup.js

+                    className='fd-tabs__item'
+                    key={child.props.id}>
+                    {this.cloneElement(child)}
+                </li>);


I think it might be better to have Tab render both the <li> and the <a> elements for each tab. I just think keeps "like" elements together. Otherwise, Tab looks like a misnamed link renderer. The map can stay here and then className and key could be moved to Tab as well.

greg-a-smith · 2019-02-19T15:34:52Z

src/Tabs/TabGroup.js

+        return React.Children.map(this.props.children, (child) => {
+            return (
+                <TabContent
+                    {...this.props.tabContentProps}


See comment below, but I think tabContentProps should be coming from child.props.

greg-a-smith · 2019-02-19T15:38:06Z

src/Tabs/TabGroup.js

+TabGroup.displayName = 'TabGroup';
+
+TabGroup.defaultProps = {
+    selectedId: '1'


I think this may be problematic. This relies on the consumer setting the id prop on the first tab to 1. Maybe this should be changed to a number prop and just simply be the index of the selected tab rather than a specific id value.

greg-a-smith · 2019-02-19T15:39:40Z

src/Tabs/TabGroup.js

+        });
+
+        if (this.props.onTabClick) {
+            this.props.onTabClick(event, id);


TIP: If you give onTabClick a default value of an empty function, you won't need to do the "if exists" check here.

greg-a-smith · 2019-02-19T15:45:48Z

src/Tabs/_TabContent.js

+TabContent.propTypes = {
+    children: PropTypes.node,
+    className: PropTypes.string,
+    id: PropTypes.string,


The id prop has no way of getting set since this is a non-exported component. The way to do it would be to include id in the tabContentProps object, but then that would end up on the <div> by virtue of the ...rest spread. The className prop is in a similar situation, however, that can stay since you need to have a way to get the fd-tabs__panel class on the <div>.

greg-a-smith

This looks pretty good. Just a few more comments and I think this will be ready to go.

greg-a-smith · 2019-02-20T13:56:36Z

src/Tabs/_TabContent.js

+import React from 'react';
+
+export const TabContent = (props) => {
+    const { children, id, selected, className, ...rest } = props;


id is no longer a prop on this component so that could be removed from here.

greg-a-smith · 2019-02-20T14:03:33Z

src/Tabs/Tab.js

+
+    // css classes used for tabs
+    const linkClasses = classnames(
+        className,


Now that this renders both the <li> and the <a>, I think className should map to the <li> and be combined with 'fd-tabs__item'. I don't think we need a linkClasses prop simply to add classes to the <a> element since they could write a selector using any attributes from the <li>.

greg-a-smith · 2019-02-20T14:23:53Z

src/Tabs/TabGroup.js

+TabGroup.displayName = 'TabGroup';
+
+TabGroup.defaultProps = {
+    selectedId: '1',


I think this prop's type needs to change to a number and represent the index of the selected tab. The reason I say that is because using id attribute values requires the use of the id prop in the Tab component and the selectedId prop in TabGroup, both of which are currently optional. It would be difficult to set a default selectedId since the tab's id value is not easily known.

The index of each tab could be bound to the onClick function being passed (in cloneElement ) so there would be no other work needed.

greg-a-smith

@chrismanciero I contributed back a couple minor edits, but I'm ready to ship this. I am now technically a partial author, so I'll just give a ⛵️ . Nice work!

* fix: Refactor Tabs component, separated controls * updated spreading props * converted TabGroup to React class component * added changes based of code review * update prop descriptions * update component based on code review * removed unneeded testing code, update unit tests * changes based on code review * Minor prop description edits

chrismanciero added 4 commits February 15, 2019 15:54

fix: Refactor Tabs component, separated controls

2dc8ea8

updated spreading props

57645c3

Merge branch 'master' into fix/Tabs-Component-refactor

9cb7fcc

converted TabGroup to React class component

7b8d729

chrismanciero requested a review from a team February 15, 2019 23:35

mobot11 changed the title ~~Fix/tabs component refactor~~ fix: tabs component refactor Feb 16, 2019

bcullman reviewed Feb 16, 2019

View reviewed changes

greg-a-smith suggested changes Feb 18, 2019

View reviewed changes

chrismanciero added 3 commits February 18, 2019 18:10

added changes based of code review

84215a9

update prop descriptions

29676a3

Merge branch 'master' into fix/Tabs-Component-refactor

59a210e

greg-a-smith suggested changes Feb 19, 2019

View reviewed changes

chrismanciero added 2 commits February 19, 2019 12:50

update component based on code review

3b61f29

removed unneeded testing code, update unit tests

84f0b4c

greg-a-smith suggested changes Feb 20, 2019

View reviewed changes

chrismanciero and others added 2 commits February 20, 2019 09:59

changes based on code review

335810f

Minor prop description edits

0d54724

greg-a-smith approved these changes Feb 20, 2019

View reviewed changes

chrismanciero merged commit 938268a into master Feb 20, 2019

chrismanciero deleted the fix/Tabs-Component-refactor branch February 20, 2019 15:29

jbadan mentioned this pull request Mar 8, 2019

Errors when inserting React node inside the paragraph tag in Tab component #401

Closed

fix: tabs component refactor #414

fix: tabs component refactor #414

Uh oh!

Conversation

chrismanciero commented Feb 15, 2019

Description

Uh oh!

bcullman Feb 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg-a-smith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg-a-smith left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bcullman Feb 16, 2019 •

edited

Loading