CC-811 improve keyboard navigation for concepts

Checklist:

• [x] I've thoroughly self-reviewed my changes
• [x] I've added tests for my changes, unless they affect admin-only areas (or are otherwise not worth testing)
• [x] I've verified any visual changes using Percy (add a commit with [percy] in the message to trigger)

Oct 30 '23 12:10 libmartinito

Test Results

    1 files  ±  0      1 suites  ±0   2m 29s :stopwatch: -42s 221 tests  - 43  220 :heavy_check_mark:  - 43  0 :zzz:  - 1  1 :x: +1  225 runs   - 44  224 :heavy_check_mark:  - 44  0 :zzz:  - 1  1 :x: +1 

For more details on these failures, see this check.

Results for commit 7b913247. ± Comparison against base commit ad9fd57e.

:recycle: This comment has been updated with latest results.

Oct 30 '23 12:10 github-actions[bot]

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (ad9fd57) 75.85% compared to head (7b91324) 73.30%. Report is 5 commits behind head on master.

@@            Coverage Diff             @@
##           master     #951      +/-   ##
==========================================
- Coverage   75.85%   73.30%   -2.56%     
==========================================
  Files         342      340       -2     
  Lines        3185     3210      +25     
  Branches      255      261       +6     
==========================================
- Hits         2416     2353      -63     
- Misses        682      764      +82     
- Partials       87       93       +6     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Oct 30 '23 12:10 codecov[bot]

not sure if i should add information about keyboard navigation and how that would look like (small notes near each button/group of buttons? one info icon with a tooltip at the start?). or should we just let users freely discover the shortcut?

Oct 30 '23 13:10 libmartinito

not sure if i should add information about keyboard navigation and how that would look like (small notes near each button/group of buttons? one info icon with a tooltip at the start?). or should we just let users freely discover the shortcut?

Yeah, good idea to highlight this - Let's do a separate PR for highlighting shortcuts and keep this focused on the functionality.

Issues I ran into:

(a) If you click enter enough times to get to a question, then you try to press an arrow key, the "submit" button is highlighted but no option is selected

https://github.com/codecrafters-io/frontend/assets/3893573/1ee1e36b-4a6e-449c-b211-9e536ca090dd

(b) Same thing happens if try to do "backspace" to step back from a question - the submit button is highlighted but no option is selected (it should instead remove the question and go back to the previous block)

(c) When on a question, just hitting "Enter" without selecting an option seems to select option C for me. I think the cleaning behaviour here would be to trigger the "Show explanation" flow?

Haven't taken a look at the code yet, will do once these issues are resolved.

Also looks like the scrolling isn't smooth in some cases:

https://github.com/codecrafters-io/frontend/assets/3893573/74f502cb-2aa5-4692-86c8-97401475c3a1

(first new block was smooth scrolled, the next was jumped to)

Oct 30 '23 14:10 rohitpaulk

(a) fixed (b) fixed. since we're adding a 'continue or step back' block after submitting an answer, i think that pressing backspace to return to the previous block should only work after it was added. i think it would be more consistent with the other blocks (c) fixed

when testing, i found that smooth scrolling doesn't work if the added element is already in view. i think that was the intention. let me know if there are other instances where it looks broken

https://github.com/codecrafters-io/frontend/assets/62695092/50d64be1-1e07-491e-a698-b704140a28a7

Oct 31 '23 08:10 libmartinito

when testing, i found that smooth scrolling doesn't work if the added element is already in view.

That's intentional, yes. In the video I shared, I was referring to the next block ("Mutability") - looks like that was hard-scrolled in? Feel free to verify if this behaviour is on master too - if so we can open a separate PR to tackle it.

Oct 31 '23 11:10 rohitpaulk

Here's a comparison showing the hard-scroll:

https://github.com/codecrafters-io/frontend/assets/3893573/cdbcd818-6707-4a99-b657-4b2b92ee5838

Oct 31 '23 13:10 rohitpaulk

updates:

• added indicator for focus state for buttons
• removed backspace for returning to previous block group to keep consistent, tab to move to button + enter or space
• removed relying on focus for question card blocks, added event listeners to document. tracking latest question card block to avoid firing for old question card blocks.

Nov 01 '23 12:11 libmartinito

for the hard-scroll, i can't replicate it after recently testing on master vs vercel deployments. maybe this could be a separate task if it persists?

Nov 01 '23 12:11 libmartinito

added indicator for focus state for buttons

@libmartinito don't think this is user-friendly enough - I mean it's good to know that buttons are focused, but with apps like ours where there are only a few interactions possible, its more common for keyboard shortcuts to be global (applied on the document).

Here's what typeform does for example (doesn't rely on focus states):

Also think the way I approached speccing this isn't optimal - maybe we should've tried an approach where you spec out the interaction in Slab/Linear first, we agree (before the sprint), and then start work on the implementation.

I'll keep this in mind for future tasks that aren't fully spec-ed.

In the interest of time, I'll start with a suggestion for the exact behaviour we could use here. Let me know if this sounds reasonable, and if there are any changes you'd make.

Here's what I've got so far: https://codecrafters.slab.com/posts/keyboard-shortcuts-for-concepts-yvfbgrta.

Don't work on the implementation just yet, let's first be sure about the spec. Try to think through edge cases - what might not be intuitive, what might not be possible to implement etc.

We can put the implementation on hold and get to it next sprint once we're clear on the spec.

Nov 01 '23 19:11 rohitpaulk