Create the menu and menuitem components in kodiak ui #36

alexanderchan · 2020-02-24T21:19:04Z

Summary

To implement the category navigation in the templates Dialog, we will need to create two new components in Kodiak UI. The Menu component will be used to display and style vertical and horizontal menus. Eventually these components would replace the components that we use to build the Jilt application nav.

Right now, we will focus on providing only a vertical menu that is used in an aside like what is in the design and will add the ability to create a horizontal menu by passing in a variant to the component.

<Menu>
  <MenuItem onClick={() => // do something}>Item 1</MenuItem>
  <MenuItem onClick={() => // do something}>Item 1</MenuItem>
</Menu>

Main Story: ch30340

###Menu props

Name	Value	Default	Required
variant	string	null	false
variantKey	string	null	false
aria-label	string	null	false

MenuItem props

MenuItem will accept all styled-system props. The isCurrent prop will add the appropriate styling for the menu item and utilize the primary theme color as the accent color.

Name	Value	Default	Required
disabled	boolean	false	false
isCurrent	boolean	false	true
onClick	() => void	null	true
variant	string	null	false
variantKey	string	null	false
as	React.ElementType	'a'	false

We will default the as prop to the anchor tag to ensure that the types always return with an href attribute if we want to render a MenuItem with an anchor tag.

Accessibility - https://www.w3.org/WAI/tutorials/menus/structure/

The Menu component should render a nav element at the root and the list should be rendered in an unordered list (

All menu components should render with a label associated with them. A string can be passed to the aria-label prop to render the following HTML.

<Menu aria-label="Main navigation">
  ...
</Menu>

// renders
<nav aria-label="Main navigation">
  <ul>
    ...
  </ul>
</nav>

The currently selected menu item should be updated for screen readers to understand which item is the currently selected item. The MenuItem component will need to support the following approaches.

Invisible text

When the MenuItem is not rendering an anchor tag, we will need to add some invisible text for the screen reader to read. This will be used when rendering a menu item for categories or a non-page change interaction.

<MenuItem as="span" isCurrent>
  <VisuallyHidden>Current Category: </VisuallyHidden>
  Product announcements
</MenuItem>

// renders
<li class="current">
  <span>
    <span class="visuallyhidden">Current Page: </span>
    Product announcements
  </span>
</li>

Using WAI-ARIA

This technique should be used when we need to render an anchor tag in the MenuItem. The aria-current="page" will indicate the current page to screen readers.

<MenuItem href="/contacts" isCurrent>
  Contacts
</MenuItem>

// renders
<li class="current">
  <a href="/contacts" aria-current="page">
    Contacts
  </a>
</li>

🚦 Acceptance criteria

Will render a vertical navigation menu with the ability to compose menu items in it
Will contain a storybook story in Kodiak
Will be fully tested
Will allow for passing anchor links or static buttons to the menu

Implementation Tasks

Deployment

Before merge to master

Note: Code merged to master should be safe to automatically deploy to production as-is.

[QA] User-testing/quality assurance done

After merge to master

Release: git push production master

alexanderchan · 2020-02-24T22:58:00Z

packages/primitives/src/Menu/MenuItem.tsx

+        {...props}
+      >
+        <li>
+          {renderAs === 'a' && (


@wesleycole I'm tempted to just take children and render them but then it's up to the calling component to set up some link defaults as well as the aria-current. It then becomes more important to look at the storybook/docs to see the sample of how it should be rendered.

Yep, as we discussed in Slack, let's just accept children for now and its up to the developer to render the inside.

wesleycole

This is looking really good just a few very small things.

wesleycole · 2020-02-25T15:02:43Z

packages/hooks/package.json

+    "prepare": "microbundle --jsx React.createElement -f es,cjs",
+    "watch": "microbundle watch --jsx React.createElement -f es,cjs"


@alexanderchan I think we can get rid of the --jsx flag here as well. I think that was a copy and paste that I did. Mind taking care of that?

wesleycole · 2020-02-25T15:02:51Z

packages/core/package.json

+    "prepare": "microbundle -f es,cjs",
+    "watch": "microbundle --watch -f es"


wesleycole · 2020-02-25T15:03:22Z

packages/primitives/src/Box/Box.tsx

+export type BaseProps = {
+  __base?: SxStyleProp
+}
+
 export function base(props: { theme: Theme; __base?: SxStyleProp }) {


Suggested change

export function base(props: { theme: Theme; __base?: SxStyleProp }) {

export function base(props: { theme: Theme } & BaseProps) {

wesleycole · 2020-02-25T16:10:24Z

packages/primitives/src/Menu/Menu.tsx

+  SystemProps
+
+export const Menu = React.forwardRef<HTMLDivElement, MenuProps>(
+  function Message({ children, variantKey = 'menu', ...props }, forwardedRef) {


Suggested change

function Message({ children, variantKey = 'menu', ...props }, forwardedRef) {

function Menu({ children, variantKey = 'menu', ...props }, forwardedRef) {

@alexanderchan can we type the ref as a ul element?

Unfortunately Box complains, we'll have to see if we can make use material-ui's approach to typing based on the as.

Oh found a way for now, I forced the type through at the Box level to be any and that fixes it so that the ref is the right type until we can do the as fix.

<Box variantKey={variantKey} variant={variant} {...props} as={renderAs} ref={forwardedRef as any} >

That works for me. Probably better for now anyway to be able to define which refs are coming in for the specific components when Box is so general.

wesleycole · 2020-02-25T16:10:36Z

packages/primitives/src/Menu/Menu.tsx

+import { Box } from '../Box'
+
+type MenuProps = {
+  children?: React.ReactNode


I think children should be required, right?

wesleycole · 2020-02-25T16:13:34Z

packages/primitives/src/Menu/MenuItem.tsx

+  SystemProps
+
+export const MenuItem = React.forwardRef<HTMLDivElement, MenuItemProps>(
+  function Message(


Suggested change

function Message(

function MenuItem(

wesleycole · 2020-02-25T16:13:43Z

packages/primitives/src/Menu/MenuItem.tsx

+
+type MenuItemProps = {
+  children: React.ReactNode
+  isCurrent?: boolean


I think isCurrent can be required.

wesleycole · 2020-02-25T16:14:52Z

packages/primitives/src/Menu/MenuItem.tsx

+  href?: string
+  rel?: HTMLAnchorElement['rel']
+  as?: React.ElementType
+} & React.HTMLAttributes<HTMLDivElement> &


Suggested change

} & React.HTMLAttributes<HTMLDivElement> &

} & React.HTMLAttributes<HTMLLIElement> &

wesleycole · 2020-02-25T16:15:04Z

packages/primitives/src/Menu/MenuItem.tsx

+  VariantProps &
+  SystemProps
+
+export const MenuItem = React.forwardRef<HTMLDivElement, MenuItemProps>(


Suggested change

export const MenuItem = React.forwardRef<HTMLDivElement, MenuItemProps>(

export const MenuItem = React.forwardRef<HTMLLIElement, MenuItemProps>(

wesleycole · 2020-02-25T17:38:01Z

packages/primitives/src/Menu/MenuItem.tsx

+        {...props}
+      >
+        <li>
+          {renderAs === 'a' && (


Yep, as we discussed in Slack, let's just accept children for now and its up to the developer to render the inside.

wesleycole

Just a few questions, overall this is looking great!

wesleycole · 2020-02-26T18:28:06Z

packages/primitives/src/Menu/Menu.tsx

+} & React.ComponentProps<typeof Box>
+
+export const Menu = React.forwardRef<HTMLUListElement, MenuProps>(function Menu(
+  { children, variantKey = 'menu', variant, ...props },


Suggested change

{ children, variantKey = 'menu', variant, ...props },

{ children, variantKey = 'menus', variant, ...props },

I'm not necessarily saying that this change needs to be made but I think we need to define whether variantKeys should be singular or plural.

Good point, yes that makes sense, plural for the variantKey 👍

wesleycole · 2020-02-26T18:29:37Z

packages/primitives/src/Menu/MenuItem.tsx

+      variantKey = 'menuitem',
+      variant,


Suggested change

variantKey = 'menuitem',

variant,

variantKey = 'menu',

variant = 'menuItem',

@alexanderchan what do you think about nesting this in the menu variantKey` similar to how we do with Dialogs?

alexanderchan added 6 commits February 24, 2020 13:08

Only bundle es modules for performance

44f9ccb

Resort by name

b6ea8d4

Export base

93e5def

Export base

540e6b4

Add base to Link

68060c9

Re-enable es for prepare

2c30bae

alexanderchan self-assigned this Feb 24, 2020

alexanderchan added 2 commits February 24, 2020 15:15

Update packages to es and watch

444b3ac

Add defaults to Link

d07fe1e

alexanderchan requested a review from wesleycole February 24, 2020 22:56

alexanderchan commented Feb 24, 2020

View reviewed changes

alexanderchan added 4 commits February 24, 2020 16:00

Update snapshot

fb25e6a

Build es and cjs

468048e

Change watch to just es

e8fa7b0

Move more code into story for docs

5fd0976

wesleycole requested changes Feb 25, 2020

View reviewed changes

alexanderchan added 10 commits February 25, 2020 17:04

Fix up forwarding

eb173db

Update Box typing

4dd4ef2

Align microbundle

296e82b

Watch only es for primitives

1be1eb5

Simplify menu component

74e9601

Update menu samples

2b81211

Sample menu

b3cfb56

Outline on link

6818508

Update spec snapshots

2e12bef

Fix codefactor extra space

196297e

alexanderchan requested a review from wesleycole February 26, 2020 18:10

alexanderchan marked this pull request as ready for review February 26, 2020 18:11

wesleycole approved these changes Feb 26, 2020

View reviewed changes

alexanderchan merged commit 4fc72b4 into master Feb 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create the menu and menuitem components in kodiak ui #36

Create the menu and menuitem components in kodiak ui #36

alexanderchan commented Feb 24, 2020

alexanderchan Feb 24, 2020

wesleycole Feb 25, 2020

wesleycole left a comment

wesleycole Feb 25, 2020

wesleycole Feb 25, 2020

wesleycole Feb 25, 2020

wesleycole Feb 25, 2020

wesleycole Feb 25, 2020

alexanderchan Feb 25, 2020

alexanderchan Feb 26, 2020 •

edited

Loading

wesleycole Feb 26, 2020

wesleycole Feb 25, 2020

wesleycole Feb 25, 2020

wesleycole Feb 25, 2020

wesleycole Feb 25, 2020

wesleycole Feb 25, 2020

wesleycole Feb 25, 2020

wesleycole left a comment

wesleycole Feb 26, 2020

alexanderchan Feb 26, 2020

wesleycole Feb 26, 2020

		"prepare": "microbundle --jsx React.createElement -f es,cjs",
		"watch": "microbundle watch --jsx React.createElement -f es,cjs"

		"prepare": "microbundle -f es,cjs",
		"watch": "microbundle --watch -f es"

	export function base(props: { theme: Theme; __base?: SxStyleProp }) {
	export function base(props: { theme: Theme } & BaseProps) {

	function Message({ children, variantKey = 'menu', ...props }, forwardedRef) {
	function Menu({ children, variantKey = 'menu', ...props }, forwardedRef) {

	} & React.HTMLAttributes<HTMLDivElement> &
	} & React.HTMLAttributes<HTMLLIElement> &

	export const MenuItem = React.forwardRef<HTMLDivElement, MenuItemProps>(
	export const MenuItem = React.forwardRef<HTMLLIElement, MenuItemProps>(

	{ children, variantKey = 'menu', variant, ...props },
	{ children, variantKey = 'menus', variant, ...props },

Create the menu and menuitem components in kodiak ui #36

Create the menu and menuitem components in kodiak ui #36

Conversation

alexanderchan commented Feb 24, 2020

Summary

MenuItem props

Accessibility - https://www.w3.org/WAI/tutorials/menus/structure/

Invisible text

Using WAI-ARIA

🚦 Acceptance criteria

Implementation Tasks

Deployment

Before merge to master

After merge to master

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleycole 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

alexanderchan Feb 26, 2020 • 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

wesleycole 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

alexanderchan Feb 26, 2020 •

edited

Loading