Add Breadcrumbs navigation component #9

edrpls · 2018-04-12T00:54:17Z

This PR will add the Breadcrumbs component, the prototype can be found here:

https://ds.gumgum.com/stable/

The component has optional submenus, please see the component's README for more details.

Make sure that the central panel in storybook has enough space (more than 991px) otherwise the component won't render (DS defaults)

edrpls · 2018-04-12T01:17:47Z

#8

edrpls · 2018-04-12T17:04:51Z

@mnicole found an issue where certain paths get repeated, I'll fix that asap

edrpls · 2018-04-12T23:36:33Z

This is ready for review again.

jscottsmith · 2018-04-13T00:41:16Z

_stories/molecules/Breadcrumbs/index.js

@@ -0,0 +1,87 @@
+import React, { Component } from 'react';
+import { select, text, boolean } from '@storybook/addon-knobs';
+import { action } from '@storybook/addon-actions';


action and text are unused imports

jscottsmith · 2018-04-13T00:44:11Z

_stories/molecules/Breadcrumbs/index.js

+    ]
+};
+
+class BreadcrumbsStory extends Component {


Can we improve this story somehow? I don't even see the breadcrumbs displaying in the storybook.

Oh, it's because of the screen size. Any way to make sure breadcrumbs display on smaller screens?

I spent like an hour yesterday trying to move the sidebar to give it more space, you'll have to make them smaller for now for it to show. (Are the notes about the width noticeable and clear enough?).

We'd have to switch either to Storybook v4 or change how we import the components.

More on that:
storybookjs/storybook#993

Oh, I completely missed the note in the README. (ohdog). I think that should be fine for now.

btw i still need to finish moving the readme to the center section so that will be more noticeable!

Btw, I made the notification larger on the readme

jscottsmith · 2018-04-13T00:54:17Z

components/molecules/Breadcrumb.jsx

+Breadcrumb.displayName = 'Breadcrumb';
+
+Breadcrumb.propTypes = {
+    LinkElt: PropTypes.oneOfType([PropTypes.element, PropTypes.func]).isRequired,


Minor, but I don't like this prop name ;). I think I prefer linkComponent or even just component. Or other options?

linkComponent sounds great, haha I didn't like it either but didn't have time to think about it.

cool, ya i think that's at least more clear. I was confused by the name at first.

jscottsmith · 2018-04-13T00:55:57Z

components/molecules/Breadcrumb.jsx

+const Breadcrumb = props => {
+    const {
+        config: { title, path, subpaths },
+        LinkElt,


also feels weird to have a capitalized prop. I'd prefer it renamed when destructured.

jscottsmith · 2018-04-13T00:58:18Z

_stories/molecules/Breadcrumbs/README.md

+| config         | Object that defines the app routes. See below for details.                                                                                                   | true     |
+| pathname       | Current pathname either from react-router or window.location.pathname                                                                                        | true     |
+| hideMenus      | Boolean attribute to prevent displaying subpath submenus                                                                                                     | false    |
+| linkElt        | Optional component to display as the breadcrumbs. Receives prop "to" as its href                                                                             | false    |


linkElt is actually LinkElt. Though I suggest changing it. See comment

It is only LinkElt on the Breadcrumb component, not on the main component. But I will apply the suggestion.

Updated to linkComponent

jscottsmith · 2018-04-13T00:59:20Z

components/molecules/Breadcrumb.jsx

@@ -0,0 +1,54 @@
+import React, { Component } from 'react';


Component is unused

jscottsmith · 2018-04-13T01:00:19Z

components/molecules/Breadcrumb.jsx

+        pathname,
+        hideMenus,
+        isLast,
+        className = ''


need className in prop types. Also, I don't think it's necessary to provide the default value ''. I think classnames will exclude it if the value is undefined.

Added proptype and default

jscottsmith · 2018-04-13T01:10:09Z

components/molecules/BreadcrumbMenu.jsx

+                            </li>
+                        ))}
+                    </ul>
+                </div>{' '}


is this {' '} dingleberry intentional? if so, maybe we should wrap it in a <span> so it at least feels intentional.

it is intentional, It was added by prettier... maybe? Good idea, the span works just fine. I'll push it.

edrpls · 2018-04-13T01:24:19Z

I'm going to make a quick change to make the config an array instead of an object, I think it makes more sense that way.

jscottsmith · 2018-04-13T01:38:12Z

components/molecules/Breadcrumb.jsx

+
+Breadcrumb.defaultProps = {
+    className: ''
+};


I don't think this is necessary when using classnames/cx. undefined values should be omitted.

jscottsmith · 2018-04-13T01:51:43Z

Not critical for merge, but a nice-to-have would be would be to make this more keyboard accessible. Specifically for breadcrumbs with sub categories. Currently focus can't trigger the pop-up menu. Seems that's the only limitation though. Might also be good to do an ARIA pass, though I don't expect it will need much since we're probably using semantic markup.

edrpls · 2018-04-13T01:56:29Z

@jscottsmith that's a good idea, but won't be possible to implement on this PR due to time constraints.

edrpls · 2018-04-13T02:47:26Z

I refactored the findTrail function a bit to better handle some cases where the root path may not be specified.

edrpls · 2018-04-13T03:30:01Z

Added an optional boolean prop (hideRoot) to hide the root element.

jscottsmith

No more comments from me. Would like to address some keyboard accessibility in the future.

mnicole · 2018-04-13T04:56:39Z

_stories/molecules/Breadcrumbs/README.md

@@ -0,0 +1,136 @@
+# Breadcrumbs
+
+**NOTE**: The component will only render with a minimun width of **991px**


tiny detail but minimun -> minimum

mnicole · 2018-04-13T05:08:59Z

components/molecules/Breadcrumbs.jsx

+                            config={path}
+                            pathname={pathname}
+                            isLast={arr.length - 1 === index}
+                            isLast={arr.length - 1 === index}


this prop is duplicated

mnicole · 2018-04-13T07:04:32Z

components/molecules/Breadcrumbs.jsx

+        // Set initial data if necessary
+        const initialData = config.path === '/' ? [initialBreadcrumb] : [];
+        // Set index difference between breadcrumbs and path
+        const pathToBreadcrumbsDiff = initialData.length ? 2 : 1;


i think the issue is here, it works with 1 : 0

I didn't see this before, I ended up changing some of the logic a bit but it should be fixed now.

edrpls · 2018-04-13T07:28:59Z

@mnicole took me a while, but I think the issues with duplicate breadcrumbs are fixed now. I may be a little woozy tomorrow.

mnicole

everything looks good now!

mnicole · 2018-04-13T15:19:56Z

components/molecules/Breadcrumbs.jsx

+        const initialSections = duplicateRoot ? parts.slice(1) : parts;
+        const maxLength = duplicateRoot ? initialSections.length + 1 : initialSections.length;
+        // Set index difference between breadcrumbs and path
+        //const pathToBreadcrumbsDiff = 2;


this got left in here! you probably want to remove line 99 too.

Add Breadcrumbs children components Add working breadcrumbs component Handle parameters and react-router integration Document breadcrumbs component Allow optional titleDecorator prop Add optional prop to hide root breadcrumb

edrpls requested review from mnicole, jscottsmith and JMSantos94 April 12, 2018 00:54

edrpls self-assigned this Apr 12, 2018

edrpls added the new component Adding a new component to the library label Apr 12, 2018

edrpls changed the base branch from master to develop April 12, 2018 00:57

edrpls mentioned this pull request Apr 12, 2018

Add a breadcrumbs component #8

Closed

edrpls removed the request for review from JMSantos94 April 12, 2018 01:13

jscottsmith reviewed Apr 13, 2018

View reviewed changes

jscottsmith approved these changes Apr 13, 2018

View reviewed changes

mnicole reviewed Apr 13, 2018

View reviewed changes

mnicole approved these changes Apr 13, 2018

View reviewed changes

Add Breadcrumbs component

7a14cd1

Add Breadcrumbs children components Add working breadcrumbs component Handle parameters and react-router integration Document breadcrumbs component Allow optional titleDecorator prop Add optional prop to hide root breadcrumb

edrpls force-pushed the feature/8-breadcrumbs-nav-component branch from 6938083 to 7a14cd1 Compare April 13, 2018 16:15

edrpls closed this Apr 13, 2018

edrpls deleted the feature/8-breadcrumbs-nav-component branch April 13, 2018 16:16

		@@ -0,0 +1,136 @@
		# Breadcrumbs

		NOTE: The component will only render with a minimun width of 991px

Add Breadcrumbs navigation component #9

Add Breadcrumbs navigation component #9

Conversation

edrpls commented Apr 12, 2018 • edited Loading

edrpls commented Apr 12, 2018

edrpls commented Apr 12, 2018

edrpls commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jscottsmith Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jscottsmith Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edrpls commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jscottsmith commented Apr 13, 2018

edrpls commented Apr 13, 2018

edrpls commented Apr 13, 2018

edrpls commented Apr 13, 2018

jscottsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edrpls commented Apr 13, 2018

mnicole left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edrpls commented Apr 12, 2018 •

edited

Loading

jscottsmith Apr 13, 2018 •

edited

Loading

jscottsmith Apr 13, 2018 •

edited

Loading