Skip to content
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

Add support for keyboard via a @useFocus argument #30

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Pjaerr
Copy link
Member

@Pjaerr Pjaerr commented Nov 12, 2024

Adds a @useFocus argument to the tooltip component (off by default) to allow showing the tooltip when a user focuses the tooltipper instead of this only being available to mouse users.

Previously this would be handled in the code of an app that uses the addon by wrapping the tooltip and manually showing/hiding it based on focus/blur events.

Added tests around the show arg as I accidentally broke this behaviour but tests didn't catch it.

Areas for review

General thoughts +

  • Does it make sense to combine the event listeners for mouse/keyboard? (ie. just use a single handler and isOverTooltipper variable), and then whether or not keyboard is supported would be handled purely by whether or not the event listeners have been added or not

  • Have I missed any places worth testing?

@Pjaerr Pjaerr requested a review from amk221 November 12, 2024 13:06
@Pjaerr Pjaerr self-assigned this Nov 12, 2024
@Pjaerr
Copy link
Member Author

Pjaerr commented Nov 12, 2024

@amk221 I've had a quick crack at this in favour of creating a wrapper component and handling in our app. Happy to go that way though if preferred.

Absolutely no rush to take a look at this, only if you get a moment!

@Pjaerr Pjaerr force-pushed the built-in-keyboard-support branch from 4cf01a7 to ccc08bd Compare November 12, 2024 13:13
@Pjaerr Pjaerr force-pushed the built-in-keyboard-support branch from ccc08bd to 84e451f Compare November 14, 2024 14:37
Comment on lines +130 to +133
triggerEvent('.tooltipper', 'focus');
triggerEvent('.tooltipper', 'blur');

await settled();
Copy link
Member Author

Choose a reason for hiding this comment

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

@amk221 I did want to use the focus/blur test helpers here but I'm really not sure why it causes the test to fail due to the tooltip not being hidden.

I wonder, could it be something to do with the fact that the focus/blur helpers actually fire focus -> focusin / blur -> focusout so potentially the order of events ends up being mixed, meaning we end up with the last event being a focus event, not a blur event. triggerEvent works as expected, I’ve updated the main focusing test to also blur, ensuring the focusing behaviour is working.

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.

1 participant