feat: fix keyboard nav for minimap#2730
Conversation
| this.primaryScroll(event); | ||
| } | ||
|
|
||
| private onWrapperFocus(): void { |
There was a problem hiding this comment.
Shouldn't onFocusWrapper and onBlurWrapper be wrappers for onFocus and onBlur, rather than for onWrapperFocus and onWrapperBlur? These names imply we're focusing/blurring a wrapper, not wrapping a function.
Either way, I'm not going to gate check-in on naming, just found it a bit confusing.
There was a problem hiding this comment.
haha it's actually both. the container we're putting this event on is minimapWrapper. the actual focus handler is called onWrapperFocus which is accurate (it's what happens when you focus the wrapper). the thing we pass to Events.bind is called onFocusWrapper which matches what the other handlers are called, because they are a wrapper around the onFocus event.
i'm going to rename onWrapperFocus to onMinimapFocus and leave the other one alone to match the existing pattern
There was a problem hiding this comment.
(the reason I didn't just do onFocus and onBlur is because those are names in the FocusableTree interface and i wanted to not imply that that's how those events worked)
2 participants
The basics
The details
Resolves
Fixes buggy minimap keyboard nav where you could keyboard nav into that tiny workspace
Proposed Changes
Reason for Changes
accessibility!
Test Coverage
added and manually tested
Documentation
updated readme
Additional Information