Add support for passing in multiple elements

[cgood92]

Add support for passing in multiple elements

Note that only passing in a single element is still supported

Features and Bug Fixes

• Test coverage added/updated.
• Typings added/updated.
• README updated (API changes, instructions, etc.).
• Changeset added (run yarn changeset locally to add one, follow prompts).

---

Demo 1

Demo 2

[changeset-bot]

🦋 Changeset detected

Latest commit: 7e26ac7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
focus-trap	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[stefcameron]

@cgood92 Wow, this is interesting! Thank you. I haven't reviewed yet, but my first thought is I wonder if this would fix #199. WDYT?

[stefcameron]

@zioth WDYT about this? Do you think it could be used to solve your use case in #199?

[zioth]

@zioth WDYT about this? Do you think it could be used to solve your use case in #199?

I think so!

[cgood92]

After this PR is merged, this PR will be good to go: focus-trap/focus-trap-react#179, after a version bump of course.

[stefcameron]

@cgood92 Nice work! My comments are mostly geared to crossing some Ts and dotting some Is, and maybe simplifying a bit of the code, but overall, I think the change is pretty solid!

Now I'm thinking of #199 in context of this change. I think that for the use case described there about the notification while in a modal, we would need some way that an existing trap can be given a new set of container elements, possibly entirely different from that with which it was initial created.

I believe that's what you've enabled with the new updateContainerElements() method on the trap object. Is that correct?

[cgood92]

@stefcameron thank you very much for the timely review. I appreciate the comments. I have gone through and made every adjustment asked for.

If it is all right, not at all to be contrary or rude, I may follow up on a few of the comments, just to shed a different perspective - all for the sake of introducing to new ideas, not at all to banter or anything. Again, I've already made the changes, so I'm not arguing for their need or not, just giving some different opinions, that is all.

Let me know if there is anything else I can do! At Workfront, we are using this component, and would love to update as soon as we can.

[stefcameron]

@cgood92

@stefcameron thank you very much for the timely review. I appreciate the comments. I have gone through and made every adjustment asked for.

Thank you! 😄 Checking this out... 👀

If it is all right, not at all to be contrary or rude, I may follow up on a few of the comments, just to shed a different perspective - all for the sake of introducing to new ideas, not at all to banter or anything. Again, I've already made the changes, so I'm not arguing for their need or not, just giving some different opinions, that is all.

Certainly, I appreciate the different perspective!

Let me know if there is anything else I can do! At Workfront, we are using this component, and would love to update as soon as we can.

Cool! Hey, Adobe is acquiring Workfront, nice. Well, assuming you like Adobe. I worked for Adobe for 10 years. Great company!

[stefcameron]

Thanks for adding that additional test case, as well as providing your perspective on use of multiple variables. I can appreciate how it can help with readability.

One comment I'd like your thoughts on...

[stefcameron]

@cgood92 I saw an issue with styling in the multi-trap demo, and also thought we could enhance the demos to show off this functionality a bit better with some highlights, so I just pushed 4bc72f0 to your fork. We can revert if you don't like it, but here are the befores and afters:

Single trap before

Single trap after

Multi trap before

Multi trap after

No functional changes to anything, though.

[stefcameron]

I have to say, this change is looking solid! I think we can declare it done after fixing the returns from pause() and unpause(). I'll do a release right after.

[cgood92]

Looks all good to go. Thanks! @stefcameron!

[stefcameron]

This looks solid to me! 💪

[stefcameron]

Fixes #199

[stefcameron]

@all-contributors please add @cgood92 for code, docs, examples, tests

[allcontributors]

@stefcameron

I've put up a pull request to add @cgood92! 🎉

[stefcameron]

Published in 6.2.0