reach / reach-ui Goto Github PK

On mobile devices, the dialog is too thin, and the button overflows.
width: 80%; instead of width: 50vw; seems to do the trick while looking good on both mobile and desktop

it is "menu" should be "menu-button"

Just an FYI that the docs for didUpdate on https://ui.reach.tech/component-component#didupdate say that it takes a nextProps and nextState rather than prevProps and prevState

I opened a PR to fix the few ESLint errors remaining in the project so we're good for now but it got me thinking: maybe we should set up a pre-push hook for ESLint to ensure that the code being pushed always matches our linting rules?

This is fairly trivial to set up using husky and lint-staged, would be more than happy to do it if people think it is a good idea.

What do you people think?

The id is auto generated for the menu button, it needs to:

• Accept a props.id before autogenerating one
• Generate ids in a deterministic way so that rehydrating a server render works (I think it ought to be broken right now)

If you've got a NavLink NVDA doesn't count it:

<MenuList>
  <MenuItem/>
  <MenuItem/>
  <MenuLink/>
</MenuList>

It announces "List, 2 items".

I'm loving the data-attribute style hooks, but with <Menu /> I am unable to access the top level div.

https://github.com/reach/reach-ui/blob/master/packages/menu-button/src/index.js#L155-L159

The styles here seem to be adding fontFamily. I'm using Glamor so the only way I can think to override these styles is by wrapping my Menu in a div. Not the end of the day but also not ideal.

Hi!

I'm wondering if it would be a good idea to use a context for WindowSize? I guess listeners are pretty cheap and it doesn't affect performance much?

Thanks

What's the best way to inspecting the CSS styles of the open menu/menu items in Chrome? Every time I click on the inspector, the menu closes. I ended up pulling down reach, changing the isOpen state, removing the click listeners, rebuilding, and linking locally.

Am I missing something here? Is there an easier way to do this? Thanks.

I followed all of these steps

git clone [email protected]:reach/reach-ui.git
cd reach-ui
yarn install
yarn bootstrap
yarn build

And when I try to yarn start I get the following errors

ERROR in ./packages/dialog/examples/nested.example.js
Module not found: Error: Can't resolve '@reach/component-component' in '/Users/bogdansoare/Hacking/reach-ui/packages/dialog/examples'
 @ ./packages/dialog/examples/nested.example.js 2:0-51
 @ ./packages ^((?!node_modules).)*\.example\.js$
 @ ./.storybook/config.js
 @ multi ./node_modules/@storybook/react/dist/server/config/polyfills.js ./node_modules/@storybook/react/dist/server/config/globals.js (webpack)-hot-middleware/client.js?reload=true ./.storybook/config.js

ERROR in ./packages/dialog/examples/aria-hides-content.example.js
Module not found: Error: Can't resolve '@reach/component-component' in '/Users/bogdansoare/Hacking/reach-ui/packages/dialog/examples'
 @ ./packages/dialog/examples/aria-hides-content.example.js 2:0-51
 @ ./packages ^((?!node_modules).)*\.example\.js$
 @ ./.storybook/config.js
 @ multi ./node_modules/@storybook/react/dist/server/config/polyfills.js ./node_modules/@storybook/react/dist/server/config/globals.js (webpack)-hot-middleware/client.js?reload=true ./.storybook/config.js

ERROR in ./packages/dialog/examples/change-styles.example.js
Module not found: Error: Can't resolve '@reach/component-component' in '/Users/bogdansoare/Hacking/reach-ui/packages/dialog/examples'
 @ ./packages/dialog/examples/change-styles.example.js 2:0-51
 @ ./packages ^((?!node_modules).)*\.example\.js$
 @ ./.storybook/config.js
 @ multi ./node_modules/@storybook/react/dist/server/config/polyfills.js ./node_modules/@storybook/react/dist/server/config/globals.js (webpack)-hot-middleware/client.js?reload=true ./.storybook/config.js

....

I haven't used lerna before, but I think the issue is with lerna, right?

Currently when a component requires a stylesheet it does a fun trick to figure out if the developer imported the styles or not by looking for a css variable on --root.

Right now it does it all the time, but I'd like to use babel-dev-expression (or whatever is best these days) to remove the check in production. Something like:

let checkStyles;
if (__DEV__) {
  checkStyles = () => {
    // the actual code
  }
} else {
  checkStyles = () => {
    // an empty function in production
  }
}

export { checkStyles };

It may actually already work, I think I've got babel-dev-expression running. Would love for somebody to verify if we can already do this, and if not, make it so we can :D

• Bring in https://github.com/jamiebuilds/create-react-context
• Do the innerRef trick for forwardRef like we did in Reach Router

I've just been working on the project structures and the first few components, and just didn't take the time yet to do it. Happy to take a PR, or I'll get to it eventually.

Might want these things to live in @reach/utils, so we can do like:

import { createContext, forwardRef } from "@reach/utils"

So that we only have to shim/polyfill/screw around with them in one place, then we can remove them easily in the future.

Hey Ryan - love your Modal component - very sharp. This is sort of a feature request, but if you think it'd be useful, I'd be more than happy to look at implementing for you in a PR.

Basically, it would be neat if we could force our DialogOverlay to be rendered in the portal inside of a Transition group of some sort. That would allow users to add animations without needing to mess with springs and such. I can't think of any clean way to do this though. Best I can come up with would be something like

<DialogOverlay renderIn={DOv => (
  <TransitionGroup>
    <CSSTransition classNames="xyz" timeout={300} key={1}>
      {DOv}
    </CSSTransition>
  </TransitionGroup>
)}>
  <DialogContent>
    <h1>Hi</h1>

Like I said, if you think there's some implementation you'd like, let me know and I'd be happy to give it a shot.

Hey there,

I was trying out MenuButton and noticed that MenuLink doesn't take react-router's Link as component prop:

Warning: Failed prop type: Invalid prop component supplied to ForwardRef.

Is that because of what's written in the docs?

If other routers' Link component uses the React.forwardRef API, you can pass them in as well. If they don’t it won't work because we will not be able to manage focus on the element the component renders.

If so, am I missing an obvious work-around (we're not quite ready yet to go all in with reach-router)? Otherwise, would it make sense for the docs to state the component's incompatibility with the widest used router a bit more explicitly?

Thanks a lot for building this!
Nicolas

Broken example:

import { Menu, MenuButton, MenuLink, MenuList } from "@reach/menu-button";
import { Link } from "react-router-dom";

const Nav = () => (
  <Menu>
    <MenuButton>My Button</MenuButton>
    <MenuList>
      <MenuLink component={Link} to="/route">Click me!</MenuLink>
    </MenuList>
  </Menu>
);

Update: That seems to work - but might kill accessibility (?):

<MenuItem onSelect={() => history.push("/my/route")}>Click Me!</MenuItem>

I noticed that currently you cannot specify the root for the dialog to be rendered into https://github.com/reach/reach-ui/blob/master/packages/portal/src/index.js#L10

The body element is hardcoded to the component.

I would very much like to see this feature. It's very important for apps that get integrated into other apps.

Would you accept a PR implementing this?

This is the only demo modal that I've found that would actually close its open modal.

I was going to look through the source code for the site to see if I could possibly submit a PR with a fix, but I'm not seeing a repo for the site (is it staring me in the face?).

The only reason I point this out is that I actually assumed that the library didn't have this capability until I found the little nugget that mentions it at the end of the documentation!

Thanks for the library! Again if there is a public repo I'll take a stab at looking into that if I get time.

Like MenuLink, it would be nice to add a component prop on MenuButton. This would allow [abstractions through components] to be used with MenuButton; for example:

1. Styles (that don't, or can't, rely on the [data-*] approach)

<MenuButton component={MyFancyButton} theme="really-fancy" />

2. Button components that care about various events (like focus) as they relate to the MenuButton components; e.g. show a tooltip on focus.

<MenuButton component={ButtonThatDoesShitOnFocus} />

When you open the animated example at https://ui.reach.tech/dialog (very bottom of the page), when the focus is supposed to be returned to the button it scrolls the page all the way up.

My hunch is that iOS doesn't mark the activating button as document.activeElement and so then the dialog dependency focus-trap tries to return focus to the activating button but actually ends up focusing document.body.

That's my guess anyway. If that's the case, I'm not sure what we want to do about it. I'm thinking maybe we use returnFocusOnDeactivate: false if we can't identify the activating element.

<Menu>
  <MenuButton>
    Actions <span aria-hidden>▾</span>
  </MenuButton>
  <MenuList>
    <input type="checkbox" />
    <input />
  </MenuList>
</Menu>

Can I use this component as a simple dropdown ? (when the dropdown is opened and focused, pressing tab focus the inputs)

Hello! Since I've know component component, I haven't used a Class anymore. I have a doubt. How do you use listeners when using component component? What is the best approach for converting this

class Example extends React.Component {
  componentDidMount () {
    this.myListener = SomeLibrary.onAction(() => null)
  }

  componentWillUnmount () {
    this.myListener.remove()
  }
}

Certain components like menus or dialogs have elements which need to be positioned via "portals" in order to be positioned properly.

Here's an example of what a screen is supposed to look like:

But it's easy to get stuff wrong with these, you can run into bugs like:

Layer inside layer getting clipped:

Layers "jumping" on top of layers that they shouldn't:

Layers getting captured inside of other positioned elements:

Layers replacing each other when rendered:

I've long thought that React needed a good generic component that just handled this.

There's also issues of accessibility. But according to ARIA's authoring practices, the components that would use layers have very different kinds of keyboard interactions. For this reason, a11y should be implemented at the actual "widget" level.

I thought a generic component that solves this would be a useful addition to Reach.

Abstract API

function Menu() {
  return (
    <Layer offset={1}>
      ...
    </Layer>
  )
}

function Dialog() {
  return (
    <Layer offset={2}>
      ...
    </Layer>
  )
}

function App() {
  return (
    <main>
      <Menu/>
      <Dialog>
        <Menu/>
      </Dialog>
    </main>
  );
}

Which would get vaguely rendered like this (had to use web components syntax so GitHub wouldn't render syntax errors)

<main>
  ...
</main>
<x-portal>
  <div style="z-index: 1"><x-menu/></div>
  <div style="z-index: 2"><x-dialog/></div>
  <div style="z-index: 3"><x-menu/></div>
</x-portal>

What is essentially happening here is:

const { Provider, Consumer } = React.createContext(0);

function Layer(props) {
  return (
    <Consumer>
      {value => (
        <Provider value={value + props.offset}>
          {props.children(value + props.offset)}
        </Provider>
      )}
    </Consumer>
  );
}

And a ReactDOM.createPortal() mixed in there somewhere.

It would be great to get some TypeScript type definitions written for each component.

When I try to use the Dialog component in combination with Transition from react-spring (version 7), the screen jumps to the original position after closing the dialog. I'm using Chrome v71.

I've put together an example here which shows the issue. I've adapted the example to the updated react-spring API to the best of my ability. To reproduce:

1. Open the dialog
2. Quickly scroll to the bottom
3. See the screen jump

I'm open to helping out, but I'm unsure where to start and even if this is a reach-ui or react-spring issue. Might also be related to #98 and #3?

In #28 you mention adding a render callback for exposing isOpen as well as enabling exit animations. I could take a look at implementing this change.

Any known gotchas I should consider?

First of all, this is an awesome project, and I like some of the react patterns you've been advocating for!

I recently read your article on putting setState in this.state when implementing context Providers (https://medium.com/@ryanflorence/react-context-and-re-renders-react-take-the-wheel-cd1d20663647) and attempted to follow that advice using Component Component instead of a normal class-based component.

The problem I ran into was that I have no handle to setState in the initialization of the state. In other words, I'd like to turn:

class Menu extends React.Component {
  constructor() {
    super()
    this.state = {
      value: this.props.defaultValue,
      setValue: (newValue) => {
        this.setState({ value: newValue })
      }
    }
  }
  render() {
    return (
      <MenuContext.Provider value={this.state}>
        {/* other stuff */}
      </MenuContext.Provider>
    )
  }
}

into:

const Menu = () => (
  <Component 
    getInitialState={
      ({ defaultValue }, { setState }) => ({ 
        value: defaultValue,
        setValue: value => setState({ value })
      })
    )}
  >
    <MenuContext.Provider value={this.state}>
        {/* other stuff */}
      </MenuContext.Provider>
  </Component>
)

No strong opinions on whether setState should be a positional arg or part of a second extras arg. If you think this is a good idea, I'd be happy to send a PR to implement it; if you think it's a terrible idea, I'd be curious to know why :)

Could Dialog prevent scroll on body when open, or is that up to the app?

Hey, this library looks really amazing, thanks for your excellent work on this.

We were looking at trying out the dialog on one of our products and ran into an issue where the latest published version of @reach/dialog is marked in its package.json as having @reach/portal@^0.1.0 as a dependency.

On npm the latest published version of @reach/portal appears to be 0.0.2 which was published ~1 month ago. https://www.npmjs.com/package/@reach/portal

From looking at the checked in package.json for @reach/portal it appears the version has been bumped to 0.1.0 in the commit titled Publish which I'm assuming was generated by lerna.

It appears publishing is currently a manual process, so I'm unsure what commands have resulted in portal not being published.

In the Component docs, there's a link to the render prop, but there's no documentation for it.

Will probably PR it.

https://ui.reach.tech/component-component#render

For the component documentation it might be nice if there was a way to mark props as required. This could be added as a column to the Props table for each component maybe?

Is there a list of browsers that reach ui will plan on supporting?

I love the library and hope to use it on my current project in production however I noticed that the MenuButton dropdown does not seem to be working on my project.

It is not working in my app or on in the demo on the website. https://ui.reach.tech/menu-button. Once you press the button the menu does not show up. It is giving me the missing styles warning probably because ie11 does not support css variables. Not sure though if that is also messing with the data-reach-menu container that is positioned absolute.

Edge appears to be working as expected.

Thank you.

We should have a githook that runs prettier so that all code committed is formatted.

I set it up in https://github.com/reach/router with pretty-quick

Would love somebody to tackle this :)

In NVDA if you open the menu with the keyboard, close it with escape, and then try to open it again with the keyboard it won't open.

It's fine if you open, close, tab away, tab back, open.

Also this might be related: https://spectrum.chat/thread/7e8cbbd4-cebf-4555-8c6e-61f96e1354ad?m=MTUzNTM2NTA2MTc5MA==

I'm an accessibility learner, so this is more a question
I was reading about best practices around accessible menus:

The solution is plain and simple: Generally, don’t use menu, menuitem, menubar, menuitemcheckbox, or menuitemradio. Only if you build something like Google Docs and deal with what you get when you press Alt+F (or Alt+Shift+F in Firefox), these are warranted. In most likely all other cases, they are not.

http://adrianroselli.com/2017/10/dont-use-aria-menu-roles-for-site-nav.html

The discussion continued here
w3c/aria-practices#353

It seems quite clear in the latest comments of that thread that "UNLESS" you are building an "application menu" (similar to a desktop menu), then better not to use all these menu aria attributes. Because...

However, it actually creates an inconvenience by triggering screen reader to go into application mode and disable HTML navigation keys for screen reader.

If Reach UI Menu components are aiming to provide Application Menu Components, then I would expect to support all the key interactions expected in the application mode? Or this is expected to be implemented by the final user of the library?
Like this example, where up-down arrows expand submenus
https://www.w3.org/TR/wai-aria-practices-1.1/examples/menubar/menubar-1/menubar-1.html
and their JS implementation is 80% about handling keyboard interactions
https://www.w3.org/TR/wai-aria-practices-1.1/examples/menubar/menubar-1/js/MenubarItemLinks.js

Again, please, these are just naive questions, I just learned today about these issues, and you have been a long time in this space ... I found this tweet from you in 2014 :D https://twitter.com/monsika/status/483833790347739137

But it would be good to give guidance for developers interested to learn the proper way of doing accessible navigation in their apps

Running on Chromium Version 70.0.3538.77 (Official Build) Arch Linux (64-bit)

Hi, thanks for the Dialog. It works great but I face one issue. You are using focus-trap internally to block events outside the Dialog, which is nice and I understand the logic behind this. I am rendering some interactive elements inside the Dialog and some of them are also using portals for rendering (for example popups), which results them being not a child node of the Dialog and this why not active. They are visible, but not clickable. Can you suggest some solution or workaround? I didn't come up with anything. Maybe it's possible to only block click events on the elements, that are above the Dialog in the DOM tree.

I saw in the Notes.md for menu-button you have

- Ignore non-menu-items for custom rendering

I'd be happy to take a stab at this, as its a feature I need anyhow. Wanted to pick your brain around what you had in mind. Should we check child.type? First hunch is to leave any non <Menu* /> children untouched

My use case is that I want to style an arrow icon inside the MenuButton based on whether the menu is open or not.

Is this a feature that could be accepted? Can I make a pull request?

There are probably other low hanging fruit that would have greater impact but a great <Image> component would be rad. Up to about whether or not you want to do progressive loading. Regardless of that decision, I feel that the component should be able to be used everywhere (even when an image is a background):

<Image src="..." alt="..." />

// this should work as expected too
<Image src="..." alt="...">
  <div>This renders inside, on top of the image, just like RN</div>
</Image> 

The guts of the React Native Web <Image> component look pretty good from an accessibility perspective and ~100 loc for what I would consider to be features in Reach UI's scope.

https://github.com/necolas/react-native-web/blob/master/packages/react-native-web/src/exports/Image/index.js

It uses a <div> plus background image but also has a hidden image for accessibilty / right click. CSS looks fairly straight forward.

Thoughts?

Says:

Failing elements:

<button
  id="some-button-id"
  aria-haspopup="menu"
  aria-controls="some-id"
  aria-expanded="false"
  type="button"
>
  Actions <span aria-hidden="true">▾</span>
</button>

It doesn't tell me which attribute though, as far as I can tell those are all valid aria attribute values? Anybody know what it wants?

After reviewing the Reach UI - Styling, the guide is suggesting to access:

the styles file at your-app/node_modules/@reach/<package-name>/styles.css.

Include those files however you include the rest of your stylesheets.

The project we are working with is using style-components as style pattern, without other popular ways to include styles by CSS.

Without a bundler (e.g. webpack) in projects with pattern such as CSS-in-JS, what would be the suggestion to properly include the style.css file when using Reach UI components?

Thank you in advance!

I'm trying to style the menu-button component using emotion.

// Menu.js
import styled from 'react-emotion';
import {
  Menu as ReachMenu,
  MenuButton as ReachMenuButton,
  MenuList as ReachMenuList,
  MenuItem as ReachMenuItem
} from '@reach/menu-button';
import '@reach/menu-button/styles.css';

const Menu = ReachMenu;

const MenuButton = styled(ReachMenuButton)`
  background-color: #fff;
  border: 1px solid #ccc;
  padding: 6px 10px;
`;

const MenuList = styled(ReachMenuList)`
  background-color: red;
`;

const MenuItem = styled(ReachMenuItem)`
  padding: 20px;
`;

export { Menu, MenuButton, MenuList, MenuItem };

But I get this error when clicking the button Cannot read property 'selectionIndex' of undefined

You can check it out here https://codesandbox.io/s/7zmq7803k6

Hello, I would like to report an issue on the component MenuButton when navigating with a keyboard.

Component: https://ui.reach.tech/menu-button

Actual behavior:
When using the key space to validate a value the pages scroll.

Desired behavior:
When using the key space the value currently highlighted is chosen and the list is hidden.

Have a nice day !

Dialogs should have aria-label or aria-labelled-by, thinking we could help people out by requiring you render a <DialogLabel>This is the label</DialogLabel>.

If a dialog renders and doesn't find a DialogLabel inside, it could warn in the console.

Internally it could handle generating the ids for the developer and hook up the aria-labelled-by.

👋

I'd like to propose making running checkStyles() optional with some kind of opt-out. My thinking is that this will make @reach/ui easier to adopt for projects that want to benefit from the accessibility features but want to use their own styling without having to override unnecessary defaults.

As it is now the warning can be ignored since it doesn't show up in production, or it can be removed by manually setting the CSS variables used in the check, but the first is a bad idea and the second would break if the check is changed. An option to remove the warning "properly" would be nice 🙂 What do you think?

Some components reference the global document/window object directly. This means that when a page renders a React app inside of an iframe (eg. widgets or entire applications) Reach UI references the parent frame's objects instead.

The most obvious example is WindowSize, when used inside of an iframe it returns the parent window size instead of the ownerDocument's. See https://codesandbox.io/s/32q7nwojp5

I believe that other components might come with similar issues. Eg. the positioning logic of the menu or the tab trap logic.

I would love to evaluate and integrate Reach at work if it turns out to provide better solutions/accessibility than ours implementations, but without iframes-compatibility I cannot even try the components. I'd be glad to help.

(Note: React always takes this scenario into account)

I noticed you were importing the styles.css on each story to apply the global css. You should be able to accomplish the same thing using

<!-- preview-head.html -->
<style>
  body {
    font-family: -apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Oxygen-Sans,
      Ubuntu, Cantarell, Helvetica Neue, sans-serif;
  }
</style>

Side Note: I like to add the emoji fonts at the end of my font-family stack. , "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol"

I am trying to embed a reach menu-button inside of a reach dialog. This doesn't seem to work, though. The MenuList never appears.

Interestingly enough, I added a debugger to the Portal code, and when I clicked the play button inside of Chrome dev tools (not the one that appears over the window) to play through the execution, the MenuList appears!

https://codesandbox.io/s/j7lvkzwyz3

Is this a bug?