search

vigetlabs / colonel-kurtz

A Block Editor
MIT License
319 stars 33 forks source link

SwitchNav onExit prop does not exist #151

Closed tjallingt closed 5 years ago

tjallingt commented 5 years ago

The Switch component seems to refer to a non-existent prop on the SwitchNav component. https://github.com/vigetlabs/colonel-kurtz/blob/master/src/components/Switch.js#L51

As far as I can tell this prop isn't used by SwitchNav: https://github.com/vigetlabs/colonel-kurtz/blob/master/src/components/SwitchNav.js

~It might be nice to add onBlur={onExit} or something like that...~

edit: onBlur will not work... Should track whether focus is inside the <nav>.

nhunzaker commented 5 years ago

Ah... thanks!

For some background, we have a component called react-focus-trap that is used to manage focus state with dialogs/menus on CK. It looks like this could have been used on the switch menu, but it is no longer necessary because Switch listens for escape key events here:

https://github.com/vigetlabs/colonel-kurtz/blob/master/src/components/Switch.js#L118-L121

Still I checked to make sure the keyboard interaction is correct:

focus

So I think this callback can go away. Any additional concerns with focus management?

tjallingt commented 5 years ago

Wow quick reaction!

I think the prop can be removed, its just kinda annoying that the menu stays open and there is no way to close it (other than esc which i just found out about haha).

I'm not sure whether people would appreciate an extra "x" menu option to close it?

Screenshot_20190403-232050__01

(On mobile sorry)

nhunzaker commented 5 years ago

I think that could be a nice enhancement (nice finger painting too)

tjallingt commented 5 years ago

Haha thanks! I can land a PR for that tommorow (but if you want to add this yourself feel free as well)

nhunzaker commented 5 years ago

A PR would be :100: x :100:. Also happy to help clarify anything along the way.

tjallingt commented 5 years ago

I'll close this since the "issue" is fixed, gonna look into adding the button (or seeing whether the SwitchNav can close when not focused) in a bit

tjallingt commented 5 years ago

@nhunzaker I think using https://ui.reach.tech/menu-button it should be quite easy to improve the SwitchNav to close on its own when no longer focused. This does somewhat change the current behavior in that it traps focus on the menu until you "esc" when using keyboard navigation.

nhunzaker commented 5 years ago

@tjallingt just following up. That sounds like a great idea. We care quite a bit about keyboard accessibility, but there's definitely a lot of room for improvement. We should look into leaning on Reach more.