ionic-framework icon indicating copy to clipboard operation
ionic-framework copied to clipboard

fix(react): Inline overlays can be conditionally rendered

Open sean-perkins opened this issue 1 year ago • 1 comments

Pull request checklist

Please check if your PR fulfills the following requirements:

  • [x] Tests for the changes have been added (for bug fixes / features)
  • [ ] Docs have been reviewed and added / updated if needed (for bug fixes / features)
    • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
  • [x] Build (npm run build) was run locally and any changes were pushed
  • [x] Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • [x] Bugfix
  • [ ] Feature
  • [ ] Code style update (formatting, renaming)
  • [ ] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] Documentation content changes
  • [ ] Other (please describe):

What is the current behavior?

Inline overlays (modal, popover) were never configured with a framework delegate. This resulted in us removing DOM nodes through manual manipulation, instead of relying on React to clear the React component and associated DOM node.

This resulted in two problems:

  1. Developers could not conditionally render an inline modal/popover.
  2. Dynamic sibling nodes rendered before an IonModal/IonPopover that were mutated while the overlay was dismissing, would result in the same exception from 1.

Issue URL: #25590, #25775

What is the new behavior?

  • Inline overlays are configured with a framework delegate implementation that teleports the inline overlay to the root of the app and back to it's original DOM location (when dismissed)
  • Inline overlays render inside a div to prevent detached DOM nodes from throwing errors when conditionally rendering an inline overlay (e.g.: {showModal && <IonModal />})
  • IonModal and IonPopover can be conditionally rendered.
  • IonModal and IonPopover will not cause exceptions when sibling content is dynamically rendered before its node.
  • Overlays are unmounted when history push/pop/replace occurs

Does this introduce a breaking change?

  • [ ] Yes
  • [x] No

Other information

Open to feedback on naming/folder structure for Github specific issue test-cases.

Dev-build: 6.2.8-dev.11663187107.1a1cbb3f

sean-perkins avatar Jul 20 '22 20:07 sean-perkins

Going to update this PR to account for the changes we identified to resolve this: #25775. Will re-add everyone for review once that work is ready.

sean-perkins avatar Aug 22 '22 17:08 sean-perkins

Closing this PR in favor of https://github.com/ionic-team/ionic-framework/pull/26111. I will keep the branch around for reference if we decide to move to React Portals in a future major version.

sean-perkins avatar Oct 14 '22 14:10 sean-perkins