feat: Nest modal support #14320
feat: Nest modal support #14320
Conversation
|
DCO Assistant Lite bot All contributors have signed the DCO. |
✅ Deploy Preview for v11-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| Focus sentinel | ||
| </span> | ||
| </div> | ||
| <Portal> |
There was a problem hiding this comment.
I think that rendering the modal in a portal by default would be a breaking change. The majority of usage in the wild probably already renders the modal via a portal and would result in nested portals. Users could/would run into not being able to access events of the innermost portal, facebook/react#14540
There was a problem hiding this comment.
One way to evaluate backwards compatibility would be to leave the existing 'with state manager' story as-is and use it as a testing grounds for how most modals are used right now.
|
The lack of focus returning to the trigger element that opened the modal (#13680) is particularly glaring when keyboard navigating through the nested modals. Figuring out a fix that is compatible with both non-nested and nested modals would be really nice. I'm not sure if it's a full blocker to landing nested modals here, but it's close. |
|
@davidmenendez Hey David - let us know if you have any other questions here! |
|
apologies. been busy! i'll be looking at this today |
|
@tay1orjones i'll continue to look at the keyboard issues. i just pushed up a change to revert the portal issue you pointed out. the story uses the original |
|
@tay1orjones whenever you have a chance to re-review it would be greatly appreciated! |
There was a problem hiding this comment.
Hey sorry for the delay. I left some comments below, I'm mainly worried about how ubiquitous modals are and if this change steps into the realm of being a breaking change.
Also I think it would be great if we could confirm support for the new launcherButtonRef functionality added in #14355 works in the nested situation.
|
Additionally I just noticed that when pressing escape with both modals open, both modals close instead of just the second one. |
|
thanks for the feedback! will work through this and reply soon |
|
@davidmenendez Hey David, just thought I'd follow up here :) |
|
apologies. i've gotten a little behind on this. still a priority. i'll have a follow up ASAP! |
|
double apologies. i'm working on this today 😬 |
|
@tay1orjones thanks for the feedback. yeah after reevaluating it i don't think the background element was necessary and i was able to make the necessary changes to get the support to work with the current implementation. the escape button should work now, but you have to remove the just wanted to get your feedback before i continue to chip away at this. |
|
Hello 👋 , thanks @davidmenendez for adding this feature! Are there any plans to merge and release this feature soon? Thanks for all the hard work on carbon 🙌 |
|
@ferdelamad seems reasonable.
@tay1orjones any thoughts? |
|
@davidmenendez Sorry for the delay here, was out for the holidays. I so appreciate your continued effort on this! In addition to the focus/onBlur fix you mention, I have a couple other thoughts:
It's really interesting that this so far has boiled down to just the |
|
@tay1orjones happy holidays! haha no worries. we've all been on cruise control these past few weeks 😂 thanks for the feedback. i totally agree with your stance on removing the story as to not promote this practice of nested modals and to include such verbiage in the documentation. that's the approach we also want to take in the C4P library. i will make the updates you suggested ASAP 👍 |
|
@tay1orjones changes made 👍 |
|
i see there's an issue with the E2E test. i'll investigate and fix ASAP 👍 |
|
E2E test pass but now percy is failing. i'm not familiar with this testing so i'll have to do some investigation. i would definitely welcome any help there haha |
There was a problem hiding this comment.
Awesome, thanks again for the work on this! I think the two remaining ideas can be contained in a new/follow-up PR if you or someone else wants to.
- That said, I think it's important to document in a new section in Modal.mdx: "Nesting of modals isn't recommended due to various UX concerns, but there should be no blockers to implementing nested modals. Here is an example" - and link out to a stackblitz essentially containing what the nested story contains right now
- I don't see why we couldn't add the fix to ComposedModal as well
tay1orjones
requested review from
tw15egan
and removed request for
andreancardona
tay1orjones
added
status: ready for review 👀
status: one more review 👀
and removed
status: waiting for author's response 💬
labels
|
yeah i can totally follow back up 👍 |
There was a problem hiding this comment.
Who knew this is all it took! Thanks for following through with this @davidmenendez 🤘🏻 🎉
688a183
* feat: nested modal support * fix: revert portal change * fix: revert modal background change and add escape * fix: reorder props * fix: add prevent default * fix: remove nested modal story * fix: revert additional changes to modal story code * fix: stopPropagation not preventDefault --------- Co-authored-by: Taylor Jones <tay1orjones@users.noreply.github.com>
Closes #14104
this is a concept for nested modal support changes. the general idea with this PR was just to demonstrate that a few small changes could make nested modal support possible. i wanted to gauge reception before continuing to integrate all the necessary testing and functionality that needs to be modified.