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

docs(nav): component playground examples #2498

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sean-perkins
Copy link
Contributor

This PR is a combination of other previously reviewed Nav playground PRs + fixing the examples to work with the revised "example component" refactor.

@vercel
Copy link

vercel bot commented Aug 17, 2022

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

Name Status Preview Updated
ionic-docs ✅ Ready (Inspect) Visit Preview Aug 17, 2022 at 4:21PM (UTC)

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 NavLink demo looks great but the modal demo still has a few glitches

Unlike Router Outlet, Nav is not tied to a particular router. This means that if we load a Nav component, and push other components to the stack, they will not affect the app's overall router. This fits use cases where you could have a modal, which needs its own sub-navigation, without making it tied to the apps URL.
<NavLinkExample />

## Navigation within a Modal
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the button is partially cut off in the Angular example:
image

Copy link
Contributor Author

@sean-perkins sean-perkins Aug 31, 2022

Choose a reason for hiding this comment

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

In React and Vue, the usage of ion-app creates a container element around the children content, creating a DOM tree like:

<ion-app class="ion-page">
  <div class="ion-page">
    <!-- component here -->
  </div>
</ion-app>

Angular does not do the same thing (likely we never noticed/wasn't an issue because ion-router-outlet handles this implementation detail).

<ion-app class="ion-page">
  <!-- component here -->
</ion-app>

Should I track this as a bug in Ionic or would we rather update our Playground demo code to workaround this issue (e.g.):

<ion-app>
  <app-example class="ion-page"></app-example>
</ion-app>

Developers using ion-router-outlet should never run into this issue.

<NavLinkExample />

## Navigation within a Modal

Copy link
Contributor

Choose a reason for hiding this comment

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

In the React example, it looks like the inner contents are not mounted until the modal is fully presented. Is that an Ionic bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct the React example uses didPresent instead of willPresent, because attempting to set the root component before the modal is presented does not work: https://stackblitz.com/edit/angular-vmakvy?file=src%2Fmain.tsx.

I can work to get a bug logged around this behavior.


## Navigation within a Modal

Modal can use Nav to offer a linear navigation that is independent of the URL.
Copy link
Contributor

Choose a reason for hiding this comment

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

The page transitions are not running in the Vue sample.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This appears to be a bug in the Vue implementation (or underlying Core bug), but I will dig in further and confirm. Seeing new page elements being constructed for each operation (pop/push/replace), but not seeing the pages unmounted or transitioned correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants