Greater-London-Authority / ldn-viz-tools

https://greater-london-authority.github.io/ldn-viz-tools/
1 stars 0 forks source link

Map controls #172

Closed PaulioRandall closed 8 months ago

PaulioRandall commented 9 months ago

What does this change?

Adds map controls including:

Also adds MapControlGroup that allows for sets of map controls to be grouped and positioned within a map.

Why?

Clickable controls for the map.

How?

I don't know.

Related issues:

None.

Does this introduce new dependencies?

No.

How is it tested?

Storybook + within an application.

How is it documented?

Storybook.

Are light and dark themes considered?

Yes but it's complicated.

jamesscottbrown commented 9 months ago

I think the uses of on:keypress={newKeyHandler(zoomIn)} are unnecessary: the button component will fire an onClick event when the user presses the Enter or Space keys while it has focus.

I agree with @mikeldn that for the pan buttons a three-row/diamond-shaped arrangement would be better than the current two-row/triangle-shaped arrangement.

I'm not entirely sure about the behaviour of the fullscreen button. It might be better to have a separate buttons for opening in full-screen and opening in the parent of the iframe (which could be auotmatically hidden if not in an iframe): as a user, I'd generally prefer to just full-screen the map, though there are use-cases for both. @mikeldn - what do you think?

PaulioRandall commented 9 months ago

I think the uses of on:keypress={newKeyHandler(zoomIn)} are unnecessary: the button component will fire an onClick event when the user presses the Enter or Space keys while it has focus.

I did not notice you omitted the on:keypress from the Button component.

Firing on:click for on:keypress is problematic in this case. While the core behavior is the same, mouse clicks change focus back to the map so MapLibre keyboard interactions continue to work without needing to click back on the map (for accessibility). Keyboard presses maintain focus on the button so it can be pressed multiple times in a row, e.g. zooming.

I figure we could check the event type expecting to get click and keypress respectively but it's always a click, i.e. MouseEvent (very confusing). However, I've discovered the event.detail is set correctly so I can use that to check the type.

PaulioRandall commented 9 months ago

We don't do enough on the accessibility front. I want to add more such enhancements but the current interface to the Button component prevents me due to lack of {...$$restProps}.

jamesscottbrown commented 8 months ago

This looks fine.