-
Notifications
You must be signed in to change notification settings - Fork 95
Don't return focus after close-after-click #3030
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
92823a3 to
be3f1f7
Compare
Signed-off-by: Raimund Schlüßler <[email protected]>
9c9545e to
6cf563b
Compare
|
It is a very good approach, thanks. |
That's true. But we can easily add a prop to the Maybe something for a follow-up if it turns out to be necessary? Or do you prefer to implement this immediately (can do so in the evening, if required)? |
I'm suspect. I am a big fan of composition over props... |
It definitely fixes the issue I am facing in the Tasks app. I would say we move forward with this solution and iterate further if we find it creates problems. |
skjnldsv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works, but I also noticed the keyboard nav is broken, when opening, the first item should be focused, it is not the case anymore.
- Open the menu
- Use arrow down to go to the second item
- See it doesn't work
You have to tab once to get into the first item THEN you can use arrows keys agai,
Indeed, but we have the same behavior on master, so not directly related to this PR. Could you open an issue, please? |
|
Yep, this is why I approved nonetheless :) |
PVince81
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| Disabled button | ||
| </ActionButton> | ||
| </Actions> | ||
| <input ref="input" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raimund-schluessler why do we have that in the documentation?
I feel this is not relevant with the ActionButton at all, no?
Feels like a corner case debug code leftover? 🤔 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that depends, one could see it as an example how to focus the input field after clicking an action. But it's true, I mainly introduced it to check that focusing actually works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured, removed in #4724
I think the example is too broad to have its place here. The nextTick and focus is common usage I would say :)
As alternative to #3028, I would like to propose a less complex solution for the
Actionscomponent focus-trap problem in #3021. When anActionhas theclose-after-clickprop set, the focus is not returned to the initial element after closing. This enables programmatically focusing another element (e.g. an input element) after theActionsmenu is closed. Checkout the example added.In difference to #3028 it does not introduce a scoped slot, hence is not a breaking change, but just keeps the current behaviour intact, without changes to the app.
On the other hand, It is a bit less flexible, since the dev cannot configure whether the focus is returned or not at the moment. But if one really needs this option, it could be implemented by simply adding a prop for it.