Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Navigation component: Store navigation tree in context #25292

Closed

Conversation

david-szabo97
Copy link
Member

@david-szabo97 david-szabo97 commented Sep 14, 2020

Fixes: #25246

Description

Adds items and menus to the Navigation context.
Example usage: Change back button label to the parent menu's title

packages/components/src/navigation/menu.js

line 31:
const { activeMenu, setActiveMenu, menus } = useNavigationContext();

line 39: 
backButtonLabel =
	parentMenu === 'root' ? backButtonLabel : menus[ parentMenu ]?.title;

Both items and menus are objects.

Keys are based on the item prop for items and they are based on menu prop for menus. This makes them easily accessible by their slug.

Values are the props of the NavigationItem and NavigationMenu items without the children prop. Items also include the menu that contains the item as menu property.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@david-szabo97 david-szabo97 added [Type] Feature New feature to highlight in changelogs. [Feature] Navigation Component A navigational waterfall component for hierarchy of items. labels Sep 14, 2020
@david-szabo97 david-szabo97 self-assigned this Sep 14, 2020
@github-actions
Copy link

github-actions bot commented Sep 14, 2020

Size Change: +368 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/components/index.js 202 kB +365 B (0%)
build/edit-post/index.js 305 kB +3 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.41 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 8.52 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 128 kB 0 B
build/block-editor/style-rtl.css 11 kB 0 B
build/block-editor/style.css 11 kB 0 B
build/block-library/editor-rtl.css 8.67 kB 0 B
build/block-library/editor.css 8.67 kB 0 B
build/block-library/index.js 139 kB 0 B
build/block-library/style-rtl.css 7.59 kB 0 B
build/block-library/style.css 7.59 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.8 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.4 kB 0 B
build/compose/index.js 9.67 kB 0 B
build/core-data/index.js 12.2 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.55 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.48 kB 0 B
build/edit-navigation/index.js 10.7 kB 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/edit-post/style-rtl.css 6.24 kB 0 B
build/edit-post/style.css 6.22 kB 0 B
build/edit-site/index.js 19.3 kB 0 B
build/edit-site/style-rtl.css 3.13 kB 0 B
build/edit-site/style.css 3.13 kB 0 B
build/edit-widgets/index.js 12.2 kB 0 B
build/edit-widgets/style-rtl.css 2.55 kB 0 B
build/edit-widgets/style.css 2.55 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/index.js 45.3 kB 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.8 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.31 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action


setNavigationItems( items );
setNavigationMenus( menus );
}, [] );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Children might change after mount (e.g. dynamically fetched menu items), so we can't use the empty dependency array here.
But children is not a primitive, so using it as a dependency would run useEffect at every render, which might be a performance problem.

I'm not sure how to solve this conundrum, though. 🤔

};

return innerRecursion( initialChildren, null );
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll happily leave someone else to review the recursion. 😄

@Copons
Copy link
Contributor

Copons commented Sep 15, 2020

I think this is getting to a good place.
I'd rather have a second opinion on the recursion, but otherwise this is (almost!) ready to be used to start the development of dependent issues in sub-branches.

Some other points of discussions:

  • What do you think of adding some get utilities?
    Something like getNavigationTreeItem and getNavigationTreeMenu.
    It would abstract the implementation and make the sub-branches’ lives easier in case of big changes to the tree.

  • This would be the big change, but it might not be needed. 🤔
    What do you think of the navigation tree as an actual tree? Meaning, with item nested into menus, etc.
    E.g.

const navigationTree = {
  menu1: {
    ...props,
    children: {
      item1: { ...props },
      item2: { ...props },
    }
  },
};
  • Currently there's nothing preventing multiple menus using the same menu.
    This is intended in order to allow showing multiple NavigationMenu in the navigation level.
    It would be an issue with the tree's menus structure though, since we are using menu as key.
    We could turn the object into an array, but either way, without a unique ID, we wouldn't have any way to get a menu.
    Should we make sure that menu are unique?

  • Should we add a group property to items?
    In my exploration of the search filtering (Navigation Component: Search Control in Menu Titles #25315), I've realized that we don't have an easy way to hide a NavigationGroup if all its items have been filtered out by the search.
    (I'm not entirely sure this would help, and anyway it's definitely something for a follow up.)

@david-szabo97
Copy link
Member Author

What do you think of adding some get utilities?
Something like getNavigationTreeItem and getNavigationTreeMenu.
It would abstract the implementation and make the sub-branches’ lives easier in case of big changes to the tree.

Agreed, we might want to change the data structure behind the scenes so an abstraction is always a good idea to keep it compatible.

What do you think of the navigation tree as an actual tree? Meaning, with item nested into menus, etc.

Nested structure is a nightmare to work with. I don't see any pros using a nested structure for storing the data. We can easily access anything we want with key-based approach. Though we could have both structure available.

Should we make sure that menu are unique?

A unique ID is a must for easy management IMO. Without that many tasks would become impossible or hacky.

Should we add a group property to items?

Yeah sure. That sounds a good idea. Though I'm not sure if you can utilise it in your PR the way you would like to do.
Groups are building blocks just like menus and items so I'll definitely add this to get a complete navigationTree

@Copons
Copy link
Contributor

Copons commented Sep 15, 2020

Should we make sure that menu are unique?

A unique ID is a must for easy management IMO. Without that many tasks would become impossible or hacky.

All right, let's assume that menu is unique right now, and decide if actually enforcing it elsewhere. 🙂

Should we add a group property to items?

Yeah sure. That sounds a good idea. Though I'm not sure if you can utilise it in your PR the way you would like to do.
Groups are building blocks just like menus and items so I'll definitely add this to get a complete navigationTree

Yeah I guess marking items with their group (if there's one) and adding the groups list in the tree could work.
(Admittedly, I haven't thought it through yet 🤔 )

Copy link
Member

@vindl vindl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the approach that you proposed in #25340 so I'll continue reviewing and commenting there. :)

@Copons
Copy link
Contributor

Copons commented Sep 17, 2020

Closing this as replaced by #25340

@Copons Copons closed this Sep 17, 2020
@Copons Copons deleted the try/navigation-navigation-tree-in-context branch September 17, 2020 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Navigation Component A navigational waterfall component for hierarchy of items. [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation Component: Store the Navigation Tree in the Context
3 participants