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

fix(props): Upgrade focus-trap to fix initialFocus prop types #390

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

sarayourfriend
Copy link
Contributor

The focus-trap library was updated in version 6.7.0 to fix an
issue with the type used for the initialFocus option where passing
false or a function that returns false was not allowed.

The relevant release is tagged here:
https://github.com/focus-trap/focus-trap/releases/tag/v6.7.0

Updating the type to include the Boolean constructor will allow
users of the Vue component to pass the false literal for the
:initial-focus prop. A function returning false already passed
the Vue type check but in previous versions of focus-trap would
raise an error. This was also fixed in 6.7.0:

focus-trap/focus-trap@14b0ee8#diff-a3ce29c1c993dbf3f968461bb3ff5e3f522d8b0c94cc2ca8f6b3ef7a9eda3621R209-R212

Updating the peer and dev dependencies to be at least 6.7.0
allows vue-focus-trap to have the correct behavior and types
and to match the latest documentation for the focus-trap library.

The `focus-trap` library was updated in version 6.7.0 to fix an
issue with the type used for the `initialFocus` option where passing
`false` or a function that returns `false` was not allowed.

The relevant release is tagged here:
https://github.com/focus-trap/focus-trap/releases/tag/v6.7.0

Updating the type to include the Boolean constructor will allow
users of the Vue component to pass the `false` literal for the
`:initial-focus` prop. A function returning `false` already passed
the Vue type check but in previous versions of `focus-trap` would
raise an error. This was also fixed in 6.7.0:

focus-trap/focus-trap@14b0ee8#diff-a3ce29c1c993dbf3f968461bb3ff5e3f522d8b0c94cc2ca8f6b3ef7a9eda3621R209-R212

Updating the peer and dev dependencies to be at least 6.7.0
allows `vue-focus-trap` to have the correct behavior and types
and to match the latest documentation for the `focus-trap` library.
@sarayourfriend
Copy link
Contributor Author

I wasn't sure how best to write a test to verify this in the cypress suite. Primiarly, I wasn't able to distinguish between the iene and aoc prop collections and where the best place to put both the false literal and the function returning false (neither of which work in the current version of vue-focus-trap) in the tests.

Super happy to update my PR with those test additions if I could get a little guidance on that from the maintainers 😀

Also, thanks for this project! We use it over on https://github.com/WordPress/openverse-frontend 🎉

Copy link
Owner

@posva posva left a comment

Choose a reason for hiding this comment

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

Thanks! I thinks it's not necessary to add a test for this

@posva posva merged commit 1af880f into posva:main Feb 2, 2022
@sarayourfriend sarayourfriend deleted the fix/initial-focus-prop branch February 2, 2022 12:36
@sarayourfriend
Copy link
Contributor Author

@posva Thanks for merging this btw. Is there any way for a new release to be cut for this change?

@posva
Copy link
Owner

posva commented Mar 10, 2022

@sarayourfriend There are a few failing tests and I haven't been able to get to them yet, if you manage to fix them and do a PR I can release a minor quickly

@sarayourfriend
Copy link
Contributor Author

sarayourfriend commented Mar 10, 2022

I'll take a look today 👍

I've been digging into it a little bit for the last 30 minutes and am struggling to follow some of the tests. I'll revisit this tomorrow morning and take a deeper look.

One thing I noticed is that it seems that for some reason initialFocus prop is getting defaulted to false rather than undefined despite not being required and not having a default 😕 Explicitly setting the default to undefined fixed at least one of the oddities I was seeing in the tests, though I'm wondering if this is expected behavior?

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.

2 participants