Add environment prop #365
Add environment prop #365
Conversation
| // https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/12733599 | ||
| canUseActiveElement = false; | ||
|
|
||
| function determineCanUseActiveElement(environment) { |
There was a problem hiding this comment.
Wrapping this into a function that runs only once (since initially canUseActiveElement is undefined) to be able to use the environment from the props instead of assuming window.
| @@ -1,6 +1,3 @@ | |||
| /** | |||
| * @jest-environment node | |||
There was a problem hiding this comment.
Allowed this tests to run on jsdom so that window can be used. All tests keep working.
There was a problem hiding this comment.
Did accessing window crash the test? Or how did you notice this?
There was a problem hiding this comment.
Yes, it was because of having the tests failing on CI: https://travis-ci.org/github/reactjs/react-tabs/builds/753987091
There was a problem hiding this comment.
I think we have to make sure that window is not required, so that tests still work without window and more important possible server-side rendering does not crash, because window is not available there.
So before accessing window there always needs to be a typeof window !== 'undefined'.
By quickly thinking about it I would try to change the code so that there is no default value and the new determineCanUseActiveElement should fallback to window if no environment given and also handle the case where environment not set and window also not available.
|
@danez Thanks for reviewing! I added the checks and reverted the Node test to make it work as before. |
|
🎉 This PR is included in version 3.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
I've ran into a use case where the focus between tabs wasn't properly handled while navigating using the arrow keys due to having
react-tabsrendered on aniframe.Inspired by
downshift, this PR adds anenvironmentprop to allow users to specify a differentwindowif they need to.