Radix-related fixes, improved main menu + OSM sign-in #465
Conversation
opyh
commented
Dec 11, 2024
opyh
commented
- Fix change requests from Add Radix UI to A11yMap #459
- Fix L hotkey (this key unlocks the OSM sign-in menu item)
- Refactor main menu and sign-in with OSM to use Radix components
Now using the correct CSS selector to display dark mode content because dark mode can be adapted by the user.
While currently unused, I guess we'll need this hook soon, so I'd not delete the file (yet).
Uses Radix UI menu component now.
There was a problem hiding this comment.
Great work, thanks a lot for your amazing PR ❤️ I really like the direction, the interface is going now. 😻
Bug: One thing that needs to be fixed though: after opening the dropdown menu either through the mouse or by focusing it and using enter/space I cannot navigate the items with the arrow keys. After pressing an arrow key, the error "TypeError: Cannot read properties of null (reading 'focus')" gets thrown. Can you take a look at this?
Discussion/Suggestion: I like that all those navigation items got moved into a dropdown menu, it makes the interface more clear and removes distractions. However, this also means that users will have a harder time finding things. This can have some negative implications especially when we want them to find some things very easily. For example when we want them to participate, e.g. the button to "add a new place". I suggest to move the "add a new place" button out of the dropdown menu again right next to menu button and also make sure it visually pops out more, e.g. by using the color "gray" for the menu button and the primary color for the "add a new place" button. Maybe we could also use an icon instead the text "add a new place". I don't know, but worth trying out. I'm curious on your opinion @opyh and @Mayaryin, what do you think? Does that make sense? Do you have other ideas?
|
Implemented the improvements you wished for (and hopefully some more!) This needs https://github.com/sozialhelden/accessibility-cloud-v2/pull/97 to work correctly (otherwise the backend does not output an This is how it looks on localhost: Responsive.main.menu.+.toolbar.mp4 |
There was a problem hiding this comment.
Hey, thanks again for your amazing work all those nice changes ❤️ Unfortunately I still found some issues. 😭
The "Events" Link is not a link but a span-tag. It also takes a few seconds after clicking before something happens without any immediate feedback. This is not accessible and causes other issues down the line. I would consider this a blocker, that needs to be changed before merging this PR.
Non-Blocker: The dropdown menu doesn't behave as expected. When opening the menu and pressing the down arrow, I would expect that the first item is highlighted. Unfortunately, that doesn't work. I can press the up-arrow and then the last item in the list is highlighted and I can navigate the list. This is a weird behaviour. It's still somewhat accessible and should not be blocking this PR, but something we should fix in the near future.
| const children = isValidating ? <Spinner /> : <Text>{label}</Text>; | ||
| const router = useRouter(); | ||
| // XXX: This should be an <AppStateAwareLink>, but this would break keyboard navigation: | ||
| // A bug in Radix UI (?) appears to not update the internal `ref` correctly. |
There was a problem hiding this comment.
Non-Blocker: Maybe the AppStateLink needs to use forwardRef in order to be able to be used with ref? Idk, my knowledge of modern react is still a bit limited, but that came to mind. Nothing that we need to solve in this PR.
There was a problem hiding this comment.
That was it! I was 100% sure I had tried this, but missed it.
| @@ -164,7 +164,7 @@ export function filterLayers( | |||
| layout: omit(localizedLayer.layout, 'line-z-offset'), | |||
| paint: omit(localizedLayer.paint, 'line-occlusion-opacity'), | |||
| } | |||
| console.log(enhancedLayer) | |||
|
Thanks for your thorough testing! I've moved the sign-in button out of the menu, added the missing Could you have another look? |
