Skip to content

Conversation

@tanner-reits
Copy link
Contributor

Issue number: resolves internal


What is the current behavior?

Some app functionality (like focus management, keyboard utils, and shimming) are tied to the ion-app which requires all Ionic applications to have a root ion-app element.

What is the new behavior?

ion-app specific init functionality is moved to the global initialize function

Does this introduce a breaking change?

  • Yes
  • No

Although it is not expected that this introduces a breaking change, these changes were introduced on the next branch as a precaution.

Other information

NOTE: This is NOT a recommended pattern for Ionic applications and was created for a specific internal use case

@vercel
Copy link

vercel bot commented Oct 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 16, 2024 6:59pm

@github-actions github-actions bot added the package: core @ionic/core package label Oct 10, 2024
*
* @default 'ion-app'
*/
appRootSelector?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For context, we needed to add this option because without ion-app, our overlay utils would default to injecting overlay element into the body of the DOM. However, this could cause problems in Frameworks like React where the actual React application was initialized inside a container in the DOM like div id="root", so if the overlay were outside that container, things like event listeners would not work.

With this option, devs can specify where the overlay should get injected using any valid CSS selector

@tanner-reits tanner-reits marked this pull request as ready for review October 10, 2024 18:27
@tanner-reits tanner-reits requested a review from a team as a code owner October 10, 2024 18:27
Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

I tested on the test app and on every React starter. I saw no issues in the test app when using the iOS and Android simulators. Passed all the tests. No issues on the starters except the list starter.

For starters, I replaced IonApp with div and IonPage with <div className='ion-page'>. As mentioned, the list starter was the only one that had issues. When I replaced IonPage with the class on the Home.tsx, it wouldn't render the list. Let me know if there's something that I'm missing.

@tanner-reits
Copy link
Contributor Author

@thetaPC Ah yes, this is another Ionic Router specific problem. The React package has a custom implementation for ion-page where it detect the presence of an Ionic Router instance. If that is the case, then it does some magic with a PageManager and React context for some lifecycle behavior. Admittedly, I'm not super familiar with what is all going on here, but if you modify the IonPage code to always think there is no Ionic Router, it results in the same behavior as if there is no ion-page element. So, I don't think this is anything we need to be concerned with since OS won't be using our router anyway.

Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

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

Tested in a browser and on iOS and Android devices. Looks good. 👍

@tanner-reits tanner-reits merged commit 15d6104 into next Oct 16, 2024
46 checks passed
@tanner-reits tanner-reits deleted the tr/ROU-11203-move-app-setup-to-initialize branch October 16, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants