gavin/maybe
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 28 Wiki Activity

Improve account modal keyboard navigation #424

Merged
sergey-tyan merged 2 commits from feature/account-hotkey-navigation-improvement into main 2024-02-12 03:17:03 +08:00
Conversation 9 Commits 2 Files Changed 2 +30 -29
sergey-tyan commented 2024-02-10 22:09:18 +08:00 (Migrated from github.com)
Copy Link

Add back button support to account keyboard navigation and autofocus text input on the form
demo video:

Add back button support to account keyboard navigation and autofocus text input on the form demo video: <video src="https://github.com/maybe-finance/maybe/assets/6569308/55b0bae6-9fa3-4616-8be8-27c99ca54281"></video>
👍 1
JoshAntBrown (Migrated from github.com) reviewed 2024-02-11 00:27:26 +08:00
JoshAntBrown (Migrated from github.com) left a comment
Copy Link

It would probably be better to move the controller further up the DOM so that the back link is included in the scope for the query for focusable links instead of changing the javascript controller to handle this edge condition of there being a back link.

Perhaps on the entire modal, or something?

It would probably be better to move the controller further up the DOM so that the back link is included in the scope for the query for focusable links instead of changing the javascript controller to handle this edge condition of there being a back link. Perhaps on the entire modal, or something?
sergey-tyan (Migrated from github.com) reviewed 2024-02-11 09:12:24 +08:00
app/javascript/controllers/list_keyboard_navigation_controller.js
@@ -20,1 +20,3 @@
return this.focusableLinks[indexOfLastFocus + direction]
let nextIndex = (indexOfLastFocus + direction) % this.focusableLinks.length
if (nextIndex < 0) nextIndex = this.focusableLinks.length - 1
sergey-tyan (Migrated from github.com) commented 2024-02-11 09:12:24 +08:00
Copy Link

if last item is focused and you press "down" then next focus will go to the first item

if last item is focused and you press "down" then next focus will go to the first item
sergey-tyan commented 2024-02-11 09:13:56 +08:00 (Migrated from github.com)
Copy Link

hey @JoshAntBrown, I added an optional attribute for passing data-controller to the modal, please take a look

hey @JoshAntBrown, I added an optional attribute for passing `data-controller` to the modal, please take a look
JoshAntBrown (Migrated from github.com) reviewed 2024-02-11 17:03:54 +08:00
app/views/accounts/new.html.erb
@@ -1,12 +1,12 @@
<h1 class="text-3xl font-semibold font-display"><%= t('.title')%></h1>
JoshAntBrown (Migrated from github.com) commented 2024-02-11 17:03:54 +08:00
Copy Link

We should update the modal helper to accept the args, and directly rather than bringing it's complexity into our view.

The modal helper is just a function at the end of the day so can accept any args we want:

<%= modal(controllers: "list-keyboard-navigation") do %>

That said, if we take this approach, I also feel like it should accept args as if it were any regular tag builder helper. This would add a fair amount of complexity to the helper as we would need to figure out how to merge it's base properties and attributes with those we pass in here.

I think we can take an even simpler approach and just render a div inside the modal that wraps all the content and applies our list-keyboard-navigation controller.

We should update the modal helper to accept the args, and directly rather than bringing it's complexity into our view. The modal helper is just a function at the end of the day so can accept any args we want: ``` <%= modal(controllers: "list-keyboard-navigation") do %> ``` That said, if we take this approach, I also feel like it should accept args as if it were any regular tag builder helper. This would add a fair amount of complexity to the helper as we would need to figure out how to merge it's base properties and attributes with those we pass in here. I think we can take an even simpler approach and just render a div inside the modal that wraps all the content and applies our list-keyboard-navigation controller.
JoshAntBrown (Migrated from github.com) reviewed 2024-02-11 17:05:34 +08:00
app/javascript/controllers/list_keyboard_navigation_controller.js
@@ -20,1 +20,3 @@
return this.focusableLinks[indexOfLastFocus + direction]
let nextIndex = (indexOfLastFocus + direction) % this.focusableLinks.length
if (nextIndex < 0) nextIndex = this.focusableLinks.length - 1
JoshAntBrown (Migrated from github.com) commented 2024-02-11 17:05:34 +08:00
Copy Link

Sounds good! Nice improvement.

Sounds good! Nice improvement.
sergey-tyan (Migrated from github.com) reviewed 2024-02-11 17:07:21 +08:00
app/views/accounts/new.html.erb
@@ -1,12 +1,12 @@
<h1 class="text-3xl font-semibold font-display"><%= t('.title')%></h1>
sergey-tyan (Migrated from github.com) commented 2024-02-11 17:07:20 +08:00
Copy Link

sure, will update the pr, thanks for the feedback!

sure, will update the pr, thanks for the feedback!
sergey-tyan (Migrated from github.com) reviewed 2024-02-11 21:40:52 +08:00
app/views/accounts/new.html.erb
@@ -1,12 +1,12 @@
<h1 class="text-3xl font-semibold font-display"><%= t('.title')%></h1>
sergey-tyan (Migrated from github.com) commented 2024-02-11 21:40:52 +08:00
Copy Link

@JoshAntBrown updated, please take a look

@JoshAntBrown updated, please take a look
Shpigford commented 2024-02-12 03:17:08 +08:00 (Migrated from github.com)
Copy Link

Great job on this, @sergey-tyan!

Great job on this, @sergey-tyan!
sergey-tyan commented 2024-02-12 09:36:15 +08:00 (Migrated from github.com)
Copy Link

thank you @Shpigford!

thank you @Shpigford!
Sign in to join this conversation.
Reviewers
No Reviewers
JoshAntBrown
sergey-tyan
Labels
Clear labels
📆 Planned
Not ready to be worked on yet, but planned for future
 💻 Self Hosted only
Issues pertaining to self-hosted versions of Maybe
 ⚙️ Config
ℹ️ Needs info
Further information is requested
 📱 Mobile
Bugs exclusively happening on mobile devices
 1️⃣ High Priority
High priority issues will be worked on next
 🚨 Urgent
Issues that are urgent for app functionality
 3️⃣ Low priority
Contributions accepted, but Maybe team will not be working on this in the near term
 2️⃣ Medium Priority
Community contributions accepted, Maybe team only works on if there are no high priority items open
 ⚡ Performance
Performance issues to address
 🎨 UI / UX
Issues related to UI / UX interactions and design.
 💎 Bounty
💰 Rewarded
📊 Data issue
Issues where the app functionally works, but the data is missing or inaccurate.
 🙋 Bounty claim
$1.5K
Community
This is a great issue for community members to work on
 dependencies
Pull requests that update a dependency file
 Design
Hosted only
Hosted app only
 Required self hoster data migration
Self hosters must run data migrations to resolve this issue
 ruby
Pull requests that update Ruby code
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
gavin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gavin/maybe#424
Delete Branch "feature/account-hotkey-navigation-improvement"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?