fix: Refactor SideNavigation to remove React-Router dependency #372

jeffredodd · 2019-02-06T06:42:03Z

Description

This PR aims to resolve #115. In order to remove the dependency on React Router from Fundamentals-React (#115), we must refactor the the following components...

~~Breadcrumb (feat: Remove React Router from Breadcrumbs #294)~~
~~Menu (feat: Remove React Router from Menu #293)~~
~~Tabs (fix: Refactor tabs to remove React-Router dependency #336)~~
SideNavigation (fix: Refactor SideNavigation to remove React-Router dependency #372)

This PR is focused on SideNav and does introduce breaking changes such as...

Complete Refactor of JSX components for SideNav
Introduction of SideNavGroup, SideNavItem, and SideNavSubItems components
<SideNavList> now takes <SideNavItem> instead of accepting an object through the items prop.
<SideNavItem> no longer creates its own <Link> but instead allows it to be added via an HOC pattern.
Demotion of react-router and react-router-dom to devDependencies.

See the documentation for examples of how to use <SideNav>.

bcullman · 2019-02-06T21:43:12Z

src/SideNavigation/SideNavigation.js

+    onItemSelect: 'Callback function when user selects an item .'
+};
+
+export const SideNavItem = ({children, expandedIds = [], glyph, id, isSubItem, name, onClick, onItemSelect, selected, selectedId, url, ...props}) => {


Let's rename to SideNavListItem

bcullman · 2019-02-06T21:44:24Z

src/SideNavigation/SideNavigation.js

+    selectedId: 'The id of the selected `SideNavItem`.'
+};

+export const SideNavGroup = ({children, className, expandedIds, onItemSelect, selectedId, title, titleProps, ...props}) => {


Lets remove SideNavGroup, and add title prop to SideNavList

… SideNavSubList

greg-a-smith

I have some other minor verbiage edits, but I'll save those pending the outcome of my other comments.

greg-a-smith · 2019-02-07T19:47:56Z

src/SideNavigation/SideNavigation.js

+                        return React.cloneElement(child, {
+                            expandedIds: this.state.expandedIds,
+                            onItemSelect: this.handleSelect,
+                            selectedId: this.state.selectedId


I know this is the story to remove react-router, but since it has turned into basically a complete refactor, I'll comment on all aspects.

I'm having a hard time quantifying it, but sending state through all the child components as props seems wrong. It just seems like too much information is known by too many components. If "selected" and "expanded" state is to be kept in the parent component (SideNav), then there should be better ways to have children report back changes to it. It looks like a lot of the uses of it in child components is simply checking it to determine a boolean outcome. Shouldn't we just pass the needed boolean to the child component?

Chatted with @greg-a-smith about this one. We will have to continue tracking selectedId state however we can track expanded collapsed state of the Sub-SideNavList's at the SubNavListItem level.

greg-a-smith · 2019-02-07T19:51:22Z

src/SideNavigation/SideNavigation.js

-    render() {
-        const { items, className, ...rest } = this.props;
+SideNav.propTypes = {
+    children: PropTypes.node,


Since the structure seems to depend on the use of the other SideNav* components, should we alter the propType definitions accordingly? What about...

children: PropTypes.oneOfType([ PropTypes.instanceOf(SideNavList), PropTypes.arrayOf(PropTypes.instanceOf(SideNavList)) ])

greg-a-smith · 2019-02-07T19:56:13Z

src/SideNavigation/SideNavigation.js

+                        expandedIds: expandedIds,
+                        onItemSelect: onItemSelect,
+                        selected: selectedId === child.props.id,
+                        selectedId: selectedId


Same comment as above ... could we just pass along booleans to the child instead of the whole list?

greg-a-smith · 2019-02-07T20:03:44Z

src/SideNavigation/SideNavigation.js

+                    {title}
+                </h1>
+                {sideNavList}
+            </div>


Do we need this <div> wrapper? It seems odd that it's included when a title exists, but not otherwise. It appears both styles tied to fd-side-nav__group already exist on fd-side-nav__list and fd-side-nav__title does not require a specific parent class. Maybe just <React.Fragment> here instead?

In this case I think we should try to match the Fundamental HTML which does include this <div>.

greg-a-smith · 2019-02-07T20:04:15Z

src/SideNavigation/SideNavigation.js

 SideNavList.propTypes = {
-    items: PropTypes.array.isRequired,
-    className: PropTypes.string
+    children: PropTypes.node,


Same comment as above regarding specific instances of children.

greg-a-smith · 2019-02-07T20:05:45Z

src/SideNavigation/SideNavigation.js

+};

+SideNavListItem.propTypes = {
+    children: PropTypes.node,


Same comment as above regarding specific instances of children.

After trying to tackle this, there is not a great way in React to enforce what components are passed as children. For now we are ignoring this issue per a conversation with @greg-a-smith.

greg-a-smith · 2019-02-07T20:07:16Z

src/SideNavigation/SideNavigation.js

-    className: PropTypes.string,
-    titleProps: PropTypes.object
+SideNavSubList.propTypes = {
+    children: PropTypes.node,


Same comment as above regarding specific instances of children.

After trying to tackle this, there is not a great way in React to enforce what components are passed as children. For now we are ignoring this issue per a conversation with @greg-a-smith.

greg-a-smith · 2019-02-07T20:10:17Z

src/SideNavigation/SideNavigation.js

+    onClick: () => {}
+};
+
+export const SideNavSubList = ({children, id, onItemSelect, open, selectedId, ...props}) => {


Do we need this extra component? Semantically, isn't a sub list just another list? Maybe have an additional prop like hasParent (that could be passed in the cloneElement calls) that allows you to add different classes or do other conditional logic.

This component has been refactored out.

bcullman · 2019-02-08T21:53:39Z

src/index.js

 export { ProductTileMedia } from './Tile/Tile';
 export { SearchInput } from './SearchInput/SearchInput';
-export { SideNav, SideNavList, SideNavGroup } from './SideNavigation/SideNavigation';
+export { SideNav, SideNavList, SideNavListItem, SideNavSubList } from './SideNavigation/SideNavigation';


should SideNavSubList still be exported?

bcullman · 2019-02-09T23:04:02Z

src/SideNavigation/SideNavigation.js


 SideNavList.propDescriptions = {
-    items: 'An array of objects with keys \'id\', \'url\', \'name\', \'hasChild\', \'child\', and \'glyph\' setting the attributes of the items.'
+    hasParent: 'Automatically set to **true** when item has a parent `SideNavListItem`',


if this is set automatically, should it be exposed as a prop type?

We have an issue where internal props have to be documented or they are left with a blank description. @greg-a-smith and I chatted about this and decided to write that these props are used for internal use only. I've opened #393 which will aim to correct this issue and would remove these prop descriptions.

greg-a-smith · 2019-02-11T18:29:28Z

src/SideNavigation/SideNavigation.js

-        className
-    );
+        this.state = {
+            expanded: this.props.expanded ? this.props.expanded : false


I think we will need a getDerivedStateFromProps (previously known as componentWillReceiveProps) to keep the internal state in sync since expanded is a prop.

Corrected in ca228a2

greg-a-smith · 2019-02-11T18:30:35Z

src/SideNavigation/SideNavigation.js

        this.state = {
-            selectedItem: 'item_2',
-            itemStates: initialState
+            selectedId: props.selectedId


I think we will need a getDerivedStateFromProps (previously known as componentWillReceiveProps) to keep the internal state in sync since selectedId is a prop.

Corrected in ca228a2

greg-a-smith · 2019-02-11T20:36:32Z

src/SideNavigation/SideNavigation.js

+                        onItemSelect(e, id, hasChild);
+                        if (hasChild) {
+                            this.handleExpand();
+                        }


I found what I think is a bug, but wanted to get thoughts on it first before creating an issue. It would be a follow-up task if it was a bug since the behavior existed previously. When click on a nav item that is the expand/collapse item, it changes the selectedId, but these items themselves shouldn't actually be "selectable". They are simply used to expand and collapse the children within it.

I agree this was an issue. Pushed 33e7b51 to fix this issue and 0835d11 to fix the corresponding test.

…mentation

…SAP/fundamental-react into fix/remove-react-router-sidenav

greg-a-smith

Assuming the builds pass, I think this looks good. I am now partial author so I'll let others review my contributions, but I'll give a partial ⛵️ .

jeffredodd · 2019-02-11T21:18:50Z

I can toss on a 🚢 for @greg-a-smith's contributions! Just have to wait for Coveralls 😄

* Save state * saving state * Adding interactivity and active item * Updating Unit Tests * Downgrading React-Router-* to devDependencies * Add Prop Descriptons * Normalized SideNavGroup into SideNavList & renamed SIdeNavSubItems to SideNavSubList * Removing SubList and Refactoring in SideNavList * Remove expanded tracking from SideNav and pushing into SideNavListItem * Linting fixes * Adding focus-trap after removing it * Package-lock was not in sync * PR feedback * Updating children prop type * Fixing issue where parent items were being selectd. Also updated documentation * Correcting testing issue * Updates to prop descriptions

Jeffrey Johnson added 8 commits February 4, 2019 20:53

Save state

b82bbb0

saving state

38c36f6

Merge branch 'master' into fix/remove-react-router-sidenav

e61f349

Adding interactivity and active item

3ff7b7a

Updating Unit Tests

5dea5f7

Downgrading React-Router-* to devDependencies

08c420c

Add Prop Descriptons

0344972

Merge branch 'master' into fix/remove-react-router-sidenav

ba8f6e6

jeffredodd requested a review from a team February 6, 2019 06:42

bcullman reviewed Feb 6, 2019

View reviewed changes

Normalized SideNavGroup into SideNavList & renamed SIdeNavSubItems to…

1034088

… SideNavSubList

greg-a-smith suggested changes Feb 7, 2019

View reviewed changes

Jeffrey Johnson added 6 commits February 7, 2019 20:31

Removing SubList and Refactoring in SideNavList

3f0de70

Remove expanded tracking from SideNav and pushing into SideNavListItem

5c9d527

Merge branch 'master' into fix/remove-react-router-sidenav

32650d6

Linting fixes

51540fe

Adding focus-trap after removing it

5bc63cf

Package-lock was not in sync

732fb99

bcullman reviewed Feb 8, 2019

View reviewed changes

bcullman reviewed Feb 9, 2019

View reviewed changes

greg-a-smith reviewed Feb 11, 2019

View reviewed changes

Jeffrey Johnson added 3 commits February 11, 2019 11:39

PR feedback

2bd92be

Updating children prop type

ca228a2

Merge branch 'master' into fix/remove-react-router-sidenav

af7ce16

greg-a-smith reviewed Feb 11, 2019

View reviewed changes

Jeffrey Johnson and others added 4 commits February 11, 2019 12:48

Fixing issue where parent items were being selectd. Also updated docu…

33e7b51

…mentation

Correcting testing issue

0835d11

Updates to prop descriptions

4791166

Merge branch 'fix/remove-react-router-sidenav' of https://github.com/…

1b7ed28

…SAP/fundamental-react into fix/remove-react-router-sidenav

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

View reviewed changes

jeffredodd merged commit e6d3287 into master Feb 11, 2019

jeffredodd deleted the fix/remove-react-router-sidenav branch February 11, 2019 21:43

fix: Refactor SideNavigation to remove React-Router dependency #372

fix: Refactor SideNavigation to remove React-Router dependency #372

Uh oh!

Conversation

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

Uh oh!

Description

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!

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!

jeffredodd Feb 11, 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!

greg-a-smith left a comment

Choose a reason for hiding this comment

Uh oh!

jeffredodd commented Feb 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jeffredodd commented Feb 6, 2019 •

edited

Loading

jeffredodd Feb 11, 2019 •

edited

Loading