feat(Menu): drill-down functionality by oshchurtt · Pull Request #659 · toptal/picasso

oshchurtt · 2019-09-10T06:18:45Z

UPD: this branch name wasn't compatible with temploy, so I created another one: #684

FX-436

Description

We want to allow nested Menu and drilling down it's sub-menus.

How to test

go to Menu story and check out Drill Down example

Review

Annotate all props in component with documentation
Create examples for component
Ensure that deployed demo has expected results and good examples
Ensure that visuals specs are green See the documentation

bytasv · 2019-09-11T06:33:30Z

+        <Menu.Item onClick={handleClick} menu={cMenu}>
+          Item C
+        </Menu.Item>
+        <Menu.Item onClick={handleClick} menu={dMenu}>


Can you also check if we can remove menu item selection when drilling down? I noticed that in this example if I click to Item D in the next sub-menu I already have preselected item:

Apparently that wasn't selection. It was focus/blur thing. Solved via key handling (element get actually re-rendered as they should on update, so the focus is reset)

bytasv · 2019-09-11T06:36:57Z

+    activeMenuProps = activeChild.props.menu.props
+    children = [
+      backButton,
+      ...extendMenuItemsWithChevrons(


So I'm thinking if this shouldn't be done inside Menu.Item directly - like if you define submenu prop - Menu.Item would know itself that it needs to add chevron
WDYT?

regarding handling the chevron inside Menu.Item I would agree 👍
However, if to talk specifically about the piece you're referring to, is a bit different topic.
In there I'm re-defining children to be rendered inside the parent/root Menu.

If we transfer the logic of rendering sub-menus into Menu.Item, we'd end up with nested <Menu> which we don't want, right?

To summarize my current conclusion regarding this one: I'm going to move the chevron logic into the `Menu.Item. Let me know if that matches your point?

Yes, I think we are on the same page - the only chevron could be handled there, it makes sense to keep drill down logic inside menu (probably not even possible otherwise)

I missed that conversation, but I left the same point - to move Chevron logic inside Menu.Item 👍

Moved Chevron logic into Menu.Item. But still end up with that extending function, just renamed.

bytasv · 2019-09-11T06:40:36Z

+    /* eslint-disable react/jsx-key */
+    const backButton = (
+      <MenuItem onClick={handleBack}>
+        <Container inline flex alignItems='center' style={{ flex: 1 }}>


Do we use flex: 1 to make Container full width? If yes - I think we should address this in a container, so users wouldn't need to add inline styles like that

cc @toptal/frontend-experience

makes sense. What prop name would you suggest @bytasv ?

Maybe we could reuse pattern that we use elsewhere - using width prop with variants auto and full? Check Input component. Also could ping on a channel to get the opinion from other devs

I got rid of both containers for this case

bytasv · 2019-09-11T06:41:42Z

+      <MenuItem onClick={handleBack}>
+        <Container inline flex alignItems='center' style={{ flex: 1 }}>
+          <BackMinor16 />
+          <Container style={{ flex: 1 }}>


So I'm wondering why do we need multiple containers here? Won't it work with single flex container have both - icon and text and aligning all that in the middle?

rriiight. Just taken this from another example and didn't give it proper thought. Nice one 👍

bytasv · 2019-09-11T06:53:32Z

+  ] as ReactElement

-    return child
+  if (activeChildPath.length === 1) {


I would inverse this if probably:

if (!activeChildPath.length) { return getActiveChildRecursively( currentLevelActiveChild.props.menu.props.children, activeChildPath.slice(1) ) } return currentLevelActiveChild

bytasv · 2019-09-11T06:56:25Z

+  onClick: (arg: number) => void
 ) {
-  children = React.Children.toArray(children).map(child => {
+  return React.Children.toArray(children).map((child, idx) => {


let's give a full name index instead of idx, was a bit confusing before I realized it's index and not id of some kind

bytasv · 2019-09-11T07:08:25Z

+}

-  /*
+function getActiveChildRecursively(


I'm just wondering if storing references to children instead of the path wouldn't be an easier to understand solution?
Something like:

const menuTrail = [childrenLevel1, childrenLevel2, childrenLevel3] const activeMenu = menuTrail[menuTrail.length - 1]

That way we wouldn't need to constantly do a recursive search

cc @toptal/frontend-experience

will try it out

I went ahead and tried to implement a non-recursive solution by using refs, which sounded like a good idea to me. However I envisioned it a bit differently from what you suggest it seems.

Specifically, I didn't understand how do we compute such a menuTrail? It looks like a flat array, but the menu structure is rather tree-like. How it can be rendered into a flat array? Maybe I miss the point though.

Still I thought that we could perhaps pass a ref to the parent Menu.Item in the onClick event, rather than path, so then we could store that ref as the activeItem in the Menu state.

This plan looked viable to me, however I faced couple complexities with it:

is it a good idea to store ref in the state?

in order to sensibly maintain keys (which is required for proper handling focus/blur issue above, the one addressed earlier) we still want to know "depth", so then we'd have to calculate it still

Smth like that. Maybe I just completely missed your point @bytasv and you meant something totally different then I thought? Let me know please.

So I don't think that you are far from what I envisioned. I'm thinking like this:

We have menu with some items that have submenu

So in this case activeContent = initial menu, right?

Now we click on one of the submenus we store activeContent in menuTrail array - const menuTrail = [] and then menuTrail.push(activeContent)

Then we change activeContent to that of the submenu - activeContent = submenu

We now see submenu and by checkinig menuTrail.length we can see that when it's menuTrail.length > 0 we can render Back button together with submenu

Now if we click Back, what we need to do is pop last item ini the menuTrail and replace it with activeContent: const activeContent = menuTrail.pop()

After we pop that item from menuTrail we can see that array is empty and we are at the very top, so we don't need to render Back button anymore

Now basically if we decide to go 3-4 levels deep we would be just using flat array for that because we don't need to keep tree structure, we just need to maintain the order of the menus and by going back we just need to pop them from that array.

I don't know if that's the best approach, but at least for me, it seems like a pretty natural way to think about it and if the code can represent same - then I think it should also be easy to understand.

Now I see what you mean. Indeed, this solution sounds simple and it's actually tempting to go ahead with it. However with such approach we're going to store (arrays of) ReactNode in the state, right? And this makes me hesitate. Earlier there used to be a paragraph on reactjs.org which is quoted in this answer on SO: https://stackoverflow.com/a/47875226/2858977
From that I would conclude that it might be not a good idea to store JSX in state, because it's kind of computed, right?

I don't have strong opinion on this though, just thinking loudly. Interested to see further feedback on this.

Ok, let's go with an existing approach. I was actually thinking about it too - whether or not it's good idea to store references... Let's not block this PR anymore with this

I think the non-recursive approach, in this case, will be easier because this path is simply a stack, what @bytasv explained. To avoid storing React components in the state we can simply mark them from the beginning with the path as a key (or other prop) and then when we need to find active child we can just iterate over children tree to find a component with key === path

@denieler , I tried to go ahead with it, but soon enough ran into 2 things that I don't see any simple resolution for:

how do we form the key? We would at least need to move the logic of pushing the selected menu into the stack into Menu, right? (it's now in Menu.Item)

how do we iterate through the children tree? Unless I miss something, we'll have to iterate recursively, right?

havenchyk

As far as I remember, the approach we discussed for this ticket should have been using context and setting value of the context inside the child component. I have nothing against checking the props of the children as well.

As for adding Chevrons, maybe it's better to extend MenuItem with that option?

havenchyk · 2019-09-11T13:07:04Z

+
+    if (childElement.props.menu) {
+      child = (
+        /* eslint-disable react/jsx-props-no-spreading */


seems you can disable the only line here

As far as I remember, the approach we discussed for this ticket should have been using context and setting value of the context inside the child component. I have nothing against checking the props of the children as well.

Not quite sure how you envision this via context? Also, there might be multiple menus on the page, right? Don't you mix it with SidebarMenu by a chance?
I might be just missing the point, let me know pls.

As for adding Chevrons, maybe it's better to extend MenuItem with that option?

Yep, done that

vedrani · 2019-09-12T08:25:31Z

@oshchurtt I tried to put Drill down Menu inside Dropdown and it closes when I click on item with menu. We need to investigate and find a way how to override it.

denieler · 2019-09-12T08:39:29Z

  variant?: VariantType
 }

+export const WrappedStringMenuItemContent = withStyles(styles, {


Suggested change

export const WrappedStringMenuItemContent = withStyles(styles, {

export const StringContent = withStyles(styles, {

or maybe StringMenuItemContent?

denieler · 2019-09-12T08:40:35Z

+  name: 'MenuItem'
+})(
+  // eslint-disable-next-line react/display-name
+  forwardRef<HTMLElement, Props>(function StringMenuItem(


and there is probably no need to forward ref, this inner component can be simplified dramatically I believe 😄

denieler · 2019-09-12T08:43:38Z

+  if (activeChildPath.length) {
+    const activeChild = getActiveChildRecursively(children, activeChildPath)
+
+    /* eslint-disable react/jsx-key */


what is the issue with the key here?

linter doesn't like that I use indexes for generating them 🤔

denieler · 2019-09-12T08:46:39Z

+    const backButton = (
+      <MenuItem onClick={handleBack} key={activeChildPath.join('_') + 'back'}>
+        <BackMinor16 />
+        <WrappedStringMenuItemContent>Back</WrappedStringMenuItemContent>


I would not reuse here WrappedStringMenuItemContent and treat BackButton as a new component in this file, which has its own styles maybe even

hm, good shout. That would actually allow to get rid of that inner component

denieler · 2019-09-12T08:52:22Z

  Item: typeof MenuItem
 }

+function extendMenuItemsWithNavigation(


can we maybe instead of function create a component, something like NavigationMenuItems (we also don't need to use this component if there are no sub-menus props in the children). I believe with the 'component' approach the code becomes more clear

let me try that

denieler · 2019-09-12T08:55:00Z

+
+  if (activeChildPath.length > 1) {
+    return getActiveChildRecursively(
+      currentLevelActiveChild.props.menu.props.children,


this looks a bit overcomplicated

denieler · 2019-09-12T08:55:18Z

+    children = [
+      backButton,
+      ...extendMenuItemsWithNavigation(
+        activeChild.props.menu.props.children,


here also a bit overcomplicated

denieler · 2019-09-12T08:58:23Z

+}

-  /*
+function getActiveChildRecursively(


I think the non-recursive approach, in this case, will be easier because this path is simply a stack, what @bytasv explained. To avoid storing React components in the state we can simply mark them from the beginning with the path as a key (or other prop) and then when we need to find active child we can just iterate over children tree to find a component with key === path

denieler · 2019-09-12T09:00:36Z

+
+  let activeMenuProps = {}
+
+  if (activeChildPath.length) {


if we would keep activeChildPath.length I would add a special name here to better understand what's going on - const shouldRenderSubMenu = activeChildPath.length (or something like this)

bytasv · 2019-09-12T10:37:04Z

@oshchurtt I tried to put Drill down Menu inside Dropdown and it closes when I click on item with menu. We need to investigate and find a way how to override it.

Maybe we can use stopPropagation in such case?

vedrani · 2019-09-13T10:10:50Z

+    console.log('Menu item is clicked')
+  }
+
+  const cMenu = (


Give better names to menus

actually I can't think of better names :) Do you?

Maybe RootMenu, ParentMenu, ChildMenu

hm, hm. In the example we don't have multiple levels of nesting, we rather have multiple sibling menus if to talk about it hierarchically. I'm not quite sure I understand what you mean therefore @vedrani 🤔

vedrani · 2019-09-13T10:11:16Z

+        <Menu.Item onClick={handleClick}>Item E</Menu.Item>
+      </Menu>
+
+      <Dropdown


Divide better in UI.

Please check. Do you think it's better now?

vedrani · 2019-09-13T10:11:59Z

+  )
+
+  return (
+    <div>


Make it have smaller width like rest of examples.

Please check the updated version. I've split "default" and "wrapped by dropdown" cases so they take half of the width each.

toptal-devbot · 2019-09-16T05:34:13Z

🎉 Last commit is successfully deployed 🎉

Demo is available on:

https://picasso.toptal.net/feat-menu-drill-down/

Your davinci-bot 🚀

denieler · 2019-09-16T06:58:04Z

-export interface Props extends StandardProps, ListNativeProps {}
+export interface Props extends StandardProps, ListNativeProps {
+  /** whether or not to handle nested navigation */
+  allowNestedNavigation?: boolean


why do we need this prop?

it's for SidebarMenu. In there submenus are rendered either in accordion or staically expanded. This prop is a way to tell the Menu component to not render Back button in the submenus.

I wasn't able to come up with a better way for solving that thing with SidebarMenu.Are you?

vedrani

Temploy is not working for this branch, do you maybe know why?

vedrani · 2019-09-16T13:18:26Z

+    console.log('Menu item is clicked')
+  }
+
+  const cMenu = (


Maybe RootMenu, ParentMenu, ChildMenu

bytasv reviewed Sep 11, 2019

View reviewed changes

havenchyk reviewed Sep 11, 2019

View reviewed changes

oshchurtt added 3 commits September 12, 2019 07:07

feat(Menu): draft of drill-down functionality

22659ea

chore: move MenuItem submenu-related logic into Menu

41fcb6f

feat(Menu): drilldown basic functionality

4e9355d

oshchurtt force-pushed the feat/menu-drill-down branch from ff556d1 to 8ef45c6 Compare September 12, 2019 04:08

oshchurtt marked this pull request as ready for review September 12, 2019 04:20

oshchurtt requested a review from a team September 12, 2019 04:20

denieler reviewed Sep 12, 2019

View reviewed changes

oshchurtt commented Sep 12, 2019

View reviewed changes

Comment thread src/components/MenuItem/MenuItem.tsx

oshchurtt and others added 10 commits September 13, 2019 11:26

chore(Menu): drilldown example to demo 3rd level submenu

bfa09b8

feat(Menu): add 'back' button for nested menus

fe8ac8a

chore(Menu): improved Drilldown example

0bd8ac2

fix(Menu): polish drilldown functionality

bd9e194

chore(Menu): simplify some logic

492319b

chore(MenuItem): try fixing some of the failed tests

643c525

fix(Menu): fix some unit tests; add a test

4c21ab9

chore(Menu): added visual snapshot for nested

e8135ce

refactor(Menu): drill based on context

a1a5ad9

chore(MenuItem): a bit simplified the code

076be58

oshchurtt changed the title ~~Feat/menu drill down~~ feat(Menu): drill-down functionality Sep 13, 2019

oshchurtt force-pushed the feat/menu-drill-down branch from 155564d to 076be58 Compare September 13, 2019 08:48

vedrani reviewed Sep 13, 2019

View reviewed changes

oshchurtt added 3 commits September 13, 2019 18:51

fix(SidebarMenu): disallow nested menu navigation

265ff79

chore(Menu): fix visual snapshots and improve story layout

e8eb6c9

fix(Menu): styling for 'back' button

577d05d

denieler reviewed Sep 16, 2019

View reviewed changes

vedrani reviewed Sep 16, 2019

View reviewed changes

Comment thread src/components/Menu/story/Drilldown.example.jsx

Comment thread src/components/Menu/story/Drilldown.example.jsx

Comment thread src/components/Menu/Menu.tsx

Comment thread src/components/Menu/Menu.tsx

Comment thread src/components/MenuItem/MenuItem.tsx

oshchurtt closed this Sep 16, 2019

havenchyk deleted the feat/menu-drill-down branch October 11, 2019 15:11

	export const WrappedStringMenuItemContent = withStyles(styles, {
	export const StringContent = withStyles(styles, {

Conversation

oshchurtt commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How to test

Review

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!

bytasv Sep 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!

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!

oshchurtt Sep 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!

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!

havenchyk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oshchurtt Sep 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!

Uh oh!

Uh oh!

vedrani commented Sep 12, 2019

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!

oshchurtt commented Sep 10, 2019 •

edited

Loading

bytasv Sep 11, 2019 •

edited

Loading

oshchurtt Sep 11, 2019 •

edited

Loading

oshchurtt Sep 11, 2019 •

edited

Loading