ilyatitovich commented 5 months ago

Fixes #197

I would like to discuss it.

Added Back button for desktop view. The button is wide. Should it be square?
Added Escape key handler. It’s strict with activeElement checking so, if I right understand keyux jump only working if current element is no <body>. Now I don't see any conflict.
Updated dependency for UI elements from secondStep to backRoute. Since we have all logic for creating backRoute, maybe it’s not necessary to use two-steps.ts and we can delete it?

Checklist

[x] Don’t rush. Check all changes in PR again.
[x] Run pnpm test.
[x] Think about changing documentation.
- If you added a script to scripts/, add a comment with a description.
- If you added a new folder, add its description to the project’s README.md.
- If you added config, describe how we use this tool in the config’s comment.
- If you added something to the project’s architecture, describe it in the project’s README.md.
- Try to focus on “why?”, not “how?”.
[x] Think about testing
- If you added a feature, add unit tests.
- If you added a new state to the UI, add visual tests.
- If you fixed the bug, think about preventing bug regression in the future.
If you changed web client:
- [x] Think about moving code to core/. What code will also be useful on other platforms?
- [x] Run pnpm size and check the difference in the JS bundle size. Is it relevant to the changes? Change the limit in web/.size-limit.json if necessary.
- [x] The UI was checked in Chrome and Firefox (and Safari or Epiphany if you have them).
- [x] Think about keyboard UX. Is it easy to use the new feature with only one hand on a keyboard? Is it easy to understand what keys to press?
- [x] Think about HTML semantics.
- [x] Think about accessibility. Check a11y recommendations. Think about how screen reader users will use the tool. Is it easy to use on a screen with bad contrast?
- [x] Think about translations.
- [x] Think about right-to-left languages. What parts of the screen should be mirrored for Arabic or Hebrew languages?
If you changed English translations:
- [x] Change translation ID if you change the meaning of the text.

github-actions[bot] commented 5 months ago

Pull request auto-reviewer

Since you changed core/:

[ ] Think about making types more precise. Can you better explain data relations by type?
[ ] Think about conflict resolution. We don’t need some very smart changing merging; just 2 changes of the same item on different clients should not break the database. What if the user changes an item on one machine and removes it on another?
[ ] Think about log and storage migration.

ilyatitovich commented 5 months ago

Updated handleEscapeKey() function.

Now we use capturing phase in addEventListener that allow us to stopping propagation early in the event flow.
Then we check route and if it fast or slow we stop propagation for prevent keyux jump, because for add and categories routes is different flow and we need jump back to list + close panel, but for slow and fast routes we need just close panel.

ilyatitovich commented 5 months ago

I changed the conditions for stopping event propagation:

Added role checking - this allows us to avoid unexpected jumps to the side menu if the user clicks on a list item with the mouse and then presses Escape. Now, in this case, the focus will move to the clicked list item.
Added tagName checking to prevent jumping back if the user clicks on a list item, then clicks somewhere else on the screen, and then presses Escape to close the side panel.

Test: video

ai commented 5 months ago

Works amazing. Thanks for helping me polish this.

Hope it helps you to go deeper into keyboard UX world (feel free to ask questions about that roles)

ilyatitovich commented 5 months ago

Yes, I learned a lot :D

hplush / slowreader

Add desktop support for going back from the steps #225

Checklist