Skip to content

feat: hook autofocus first input#979

Merged
Klakurka merged 1 commit intomasterfrom
feat/auto-focus-first-input
Mar 12, 2025
Merged

feat: hook autofocus first input#979
Klakurka merged 1 commit intomasterfrom
feat/auto-focus-first-input

Conversation

@lissavxo
Copy link
Copy Markdown
Collaborator

@lissavxo lissavxo commented Mar 5, 2025

Related to #542

Description

Added a generic hook to autoFocus first input of forms in the dashboard

Test plan

  • Go to buttons page, click in the button + to add a new button, check if the first field of the form is automatically focused
  • Also check other forms like wallets page forms to add a new wallet and to edit

}, 100)
}

document.addEventListener('click', handleClick)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not working well, clicking in another field refocuses the first field.


function useAutoFocusFirstInput (): void {
useEffect(() => {
const focusFirstInput = (element = document): void => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have this default parameter and then call with it anyway, below?

@lissavxo lissavxo requested a review from chedieck March 6, 2025 14:27
@lissavxo lissavxo force-pushed the feat/auto-focus-first-input branch from e07c315 to fc6ddee Compare March 6, 2025 14:35
Klakurka
Klakurka previously approved these changes Mar 9, 2025
Copy link
Copy Markdown
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a good solution.

The idea of using an event listener to click events to focus the first input is a bad idea, clicks can have thousands of applications, and this is evident from all the bugs this PR introduces.

Some I found:

  1. Selecting a wallet in the creation button form will refocus the button name input
  2. Can't select a timezone on the account view anymore
  3. Misclicking a radio selection in the wallet edit will annoyingly refocus the first input, scrolling up the modal
  4. Ordering a column in the txs table of the button view page will focus POST URL trigger input, also annoyingly scrolling downwards if there are enough entries/tables.

In summary, the overall experience of having the first input focused after clicking anywhere is just bad UX.

I much rather that this is done cleanly having to edit all files that have some kind of form than this general solution that creates more problems than it solves.

@lissavxo
Copy link
Copy Markdown
Collaborator Author

This is not a good solution.

The idea of using an event listener to click events to focus the first input is a bad idea, clicks can have thousands of applications, and this is evident from all the bugs this PR introduces.

Some I found:

  1. Selecting a wallet in the creation button form will refocus the button name input
  2. Can't select a timezone on the account view anymore
  3. Misclicking a radio selection in the wallet edit will annoyingly refocus the first input, scrolling up the modal
  4. Ordering a column in the txs table of the button view page will focus POST URL trigger input, also annoyingly scrolling downwards if there are enough entries/tables.

In summary, the overall experience of having the first input focused after clicking anywhere is just bad UX.

I much rather that this is done cleanly having to edit all files that have some kind of form than this general solution that creates more problems than it solves.

Fair enough, I see how this can bring some issues, we don't have many modal forms so I can go in each for and autofocus the first input

Copy link
Copy Markdown
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works well, clean code.

@Klakurka Klakurka merged commit 0ea70ac into master Mar 12, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants