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

[Feat]: watchFocus option #937

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Conversation

S-Shingler
Copy link
Contributor

@S-Shingler S-Shingler commented Jul 11, 2024

Hi David,

Firstly, thank you for an awesome package!

I went more in depth regarding my issue in this discussion:

Essentially I need a way to override the default slide focus behaviour for some accessibility work I've been doing.

A watchFocus option feels natural to me but curious to hear your thoughts.

Cheers,
Sam


Closes:

Repository owner deleted a comment from Mitch-At-Work Jul 12, 2024
@davidjerleke
Copy link
Owner

Thanks for your contribution @S-Shingler 👍🏻,

I’m on vacation so I will review and test this when I’m back. Thank you for your patience.

@davidjerleke davidjerleke added the feature request New feature or request label Jul 13, 2024
@S-Shingler
Copy link
Contributor Author

No worries, enjoy your vacation!

@davidjerleke
Copy link
Owner

davidjerleke commented Jul 16, 2024

Hi @Mitch-At-Work,

Thanks for your input. I think it's better to create a separate discussion if you're unsure about how the current focus behavior is operating. By all means, what you're mentioning could be a bug but it could also be by design and you might be misunderstanding the feature.

I think it makes sense to not aim for just opting out of this feature if it's causing you trouble, but rather creating a discussion with a CodeSandbox along with clear steps to reproduce the behavior you think is problematic. This way, if it turns out to be a bug, we can fix the bug and improve the library. If not, you might better understand why it works the way it does.

With that said, this feature is on the roadmap and I will review and test this PR as soon as possible so it's upcoming:

@davidjerleke davidjerleke self-assigned this Jul 16, 2024
@davidjerleke davidjerleke changed the title feat: watchFocus option [Feat]: watchFocus option Jul 16, 2024
@davidjerleke davidjerleke added the core This is related to the core package label Jul 18, 2024
@davidjerleke
Copy link
Owner

Hi @S-Shingler,

I've made some modifications in order to align the watchFocus option structure with the other watchers like watchResize, watchDrag and watchSlides. I think this feature is ready for a release but I would appreciate if you would try it out before we do so I haven't missed anything obvious.

In you local environment, you can try any of the three playgrounds and test the new feature out:

  • yarn start:vanilla (starts the vanilla playground)
  • yarn start:react (starts the react playground)
  • yarn start:solid (starts the solid playground)

All environments live in this folder: <your_root_folder>/playgrounds/. Let me know how it goes!

Thanks in advance.

@S-Shingler
Copy link
Contributor Author

Hi @davidjerleke,

Everything is looking good.

Cheers,
Sam

Co-authored-by: Samuel Shingler <[email protected]>
Co-authored-by: David Jerleke <[email protected]>
@davidjerleke davidjerleke merged commit fe380e8 into davidjerleke:master Aug 20, 2024
@davidjerleke davidjerleke added the resolved This issue is resolved label Aug 20, 2024
@davidjerleke
Copy link
Owner

davidjerleke commented Aug 20, 2024

@S-Shingler, @Mitch-At-Work, this feature has been included in the v8.2.0 release. Thanks a lot for your contribution 💫!

@S-Shingler
Copy link
Contributor Author

@S-Shingler, @Mitch-At-Work, this feature has been included in the v8.2.0 release. Thanks a lot for your contribution 💫!

That's awesome, thank you! @davidjerleke

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core This is related to the core package feature request New feature or request resolved This issue is resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants