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(react): backdrop for inline modal/popover overlay #24453

Merged
merged 9 commits into from
Jan 7, 2022
Merged

Conversation

sean-perkins
Copy link
Contributor

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • 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:

  • 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?

In the React target, inline overlays (ion-popover and ion-modal) will not define their nested children custom elements (ion-backdrop), when in a production tree-shaken build.

Issue Number: #24449

What is the new behavior?

Inline overlays will share the same custom element definition logic as the regular overlays. This correctly defines ion-backdrop for popover/modal inline overlays.

Does this introduce a breaking change?

  • Yes
  • No

Other information

@sean-perkins sean-perkins requested a review from a team as a code owner December 20, 2021 20:17
@github-actions github-actions bot added package: core @ionic/core package package: react @ionic/react package labels Dec 20, 2021
@sean-perkins sean-perkins removed the request for review from a team December 20, 2021 20:22
@sean-perkins
Copy link
Contributor Author

I'd love to wait until after the New Year to discuss this with the team 🙂 . I could see use deciding not to expose a new public method on the controller API and instead exporting pure constants for just the popover & modal custom element definitions.

@sean-perkins
Copy link
Contributor Author

Created a dev build of the latest to test with: 6.0.1-dev.1641412400.d3fd57e & re-verified everything works as expected with the reproduction app: https://github.com/JohnDeved/ionic-popover-bug.

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

The original issue says it impacts Vue as well, so we should make this change in https://github.com/ionic-team/ionic-framework/blob/main/packages/vue/src/components/Overlays.ts#L12-L18 as well

Also, my understanding of the issue is this impacts all overlay components, not just modal and popover. If that is the case, components like action sheet also need to be updated: https://github.com/ionic-team/ionic-framework/blob/main/packages/react/src/components/IonActionSheet.tsx#L6

core/src/utils/overlays.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot added the package: vue @ionic/vue package label Jan 6, 2022
@sean-perkins
Copy link
Contributor Author

Alright this should look better now. Didn't realize the overlays generation script was local to this repo. Simplified not needing to update the output targets.

Kept the commits to follow the changes, but will squash and rework to better explain it affects react & vue targets.

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Great job!

@sean-perkins sean-perkins merged commit 77f8412 into main Jan 7, 2022
@sean-perkins sean-perkins deleted the FW-438 branch January 7, 2022 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package package: react @ionic/react package package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants