The Wayback Machine - https://web.archive.org/web/20201010042817/https://github.com/ionic-team/ionic-framework/pull/21725
Skip to content

/ ionic-framework

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

Jump to bottom

fix(nav): insertPages method correctly inserts multiple pages with props #21725

Merged
merged 7 commits into from Jul 22, 2020
Merged

fix(nav): insertPages method correctly inserts multiple pages with props #21725

merged 7 commits into from Jul 22, 2020

Conversation

@liamdebeasi
Copy link
Member

@liamdebeasi liamdebeasi commented Jul 10, 2020

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?

Issue Number: fixes #21724

What is the new behavior?

Two issues that have been fixed:

  1. insertPages now has correct types to indicate that you can insert multiple pages with props
  2. cleanup method has been fixed to account for stack items that do not have DOM elements yet.

Does this introduce a breaking change?

  • Yes
  • No

Other information
@liamdebeasi
insertPages multiple support
Loading status checks…
0685185
liamdebeasi added 2 commits Jul 10, 2020
@liamdebeasi
add test
Loading status checks…
25744f3
@liamdebeasi
remove test didnt actually test what I wanted
Loading status checks…
a519bd4
@Yohandah
Copy link

@Yohandah Yohandah commented Jul 15, 2020 •
edited

I believe the method IonNav.setPages could use the same improvements as well ? Since it is not possible to use it with props as well

And I see you named the properties of the NavComponentWithProps type page and params, wouldn't make more sense to match .setRoot and .push and name them component and componentProps ?
@Yohandah Yohandah mentioned this pull request Jul 15, 2020
Closed
@liamdebeasi
Copy link
Member Author

@liamdebeasi liamdebeasi commented Jul 15, 2020

Yeah probably. page and params is what we have been using internally for a while, but component and componentProps (or similar) makes more sense.
liamdebeasi added 4 commits Jul 15, 2020
@liamdebeasi
update types
Loading status checks…
0395082
@liamdebeasi
update setPAges
Loading status checks…
6a879eb
@liamdebeasi
fix types
Loading status checks…
a0f7603
@liamdebeasi
fix test
Loading status checks…
6c07747
@liamdebeasi liamdebeasi requested a review from brandyscarney Jul 17, 2020
@brandyscarney
brandyscarney approved these changes Jul 22, 2020
View changes
@liamdebeasi liamdebeasi merged commit eb592b8 into master Jul 22, 2020
2 checks passed
2 checks passed
build Workflow: build
Details
screenshot Screenshot
Details
@liamdebeasi liamdebeasi deleted the insert-pages-multiple branch Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandyscarney brandyscarney

Assignees
No one assigned
Labels
package: core
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

bug: setPages and insertPages do not expose proper types
3 participants
@liamdebeasi @Yohandah @brandyscarney
You can’t perform that action at this time.