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

Navigator: fix isInitial logic #65527

Merged
merged 2 commits into from
Sep 24, 2024
Merged

Navigator: fix isInitial logic #65527

merged 2 commits into from
Sep 24, 2024

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Sep 20, 2024

What?

Discovered while working on #65523

Set the isInitial flag to false in the internal Navigator reducer only when effectively navigating to a new screen.

Why?

Previously, the isInitial flag was set to false on every navigation, even the ones that were not leading to effectively navigating to a new screen. This was causing unwanted side effects, like the one flagged in #65523

How?

The isInitial flag is set to false only when navigating to a new location.

Testing Instructions

In Storybook:

  • Apply the following diff
diff --git a/packages/components/src/navigator/stories/index.story.tsx b/packages/components/src/navigator/stories/index.story.tsx
index 30b9c71a36..8d4881a563 100644
--- a/packages/components/src/navigator/stories/index.story.tsx
+++ b/packages/components/src/navigator/stories/index.story.tsx
@@ -64,6 +64,10 @@ export const Default: StoryObj< typeof NavigatorProvider > = {
 					<h2>This is the home screen.</h2>
 
 					<VStack alignment="left">
+						<NavigatorButton variant="primary" path="/">
+							Go to home screen.
+						</NavigatorButton>
+
 						<NavigatorButton variant="primary" path="/child">
 							Go to child screen.
 						</NavigatorButton>
  • Load the default Storybook example for Navigator
  • Click on the "Go to home screen" button
  • Make sure the screen doesn't animate

In the editor:

  • Open site editor
  • Open the global styles sidebar
  • Make sure the initial screen doesn't animate

Screenshots or screencast

Before (trunk) After (this PR)
Kapture.2024-09-20.at.14.24.56.mp4
Kapture.2024-09-20.at.14.26.11.mp4

@ciampo ciampo self-assigned this Sep 20, 2024
@ciampo ciampo requested a review from a team September 20, 2024 12:53
@ciampo ciampo added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Sep 20, 2024
@ciampo ciampo marked this pull request as ready for review September 20, 2024 12:55
@ciampo ciampo requested a review from ajitbohra as a code owner September 20, 2024 12:55
Copy link

github-actions bot commented Sep 20, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ciampo <[email protected]>
Co-authored-by: tyxla <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks for the fix 🚀

@ciampo ciampo force-pushed the fix/navigator-is-initial branch from 652b56a to 014f023 Compare September 23, 2024 14:42
@ciampo ciampo enabled auto-merge (squash) September 23, 2024 14:43
@tyxla
Copy link
Member

tyxla commented Sep 23, 2024

The failing tests seem unrelated FWIW.

@ciampo ciampo force-pushed the fix/navigator-is-initial branch from 014f023 to 65baea0 Compare September 24, 2024 08:41
@ciampo ciampo disabled auto-merge September 24, 2024 09:42
@ciampo
Copy link
Contributor Author

ciampo commented Sep 24, 2024

Yeah, the e2e test failures are about block bindings and can be seen across many other PRs, so they they're not related. All e2e and unit tests related to these changes are passing, so I'm going to merge anyway.

@ciampo ciampo merged commit efd622d into trunk Sep 24, 2024
59 of 61 checks passed
@ciampo ciampo deleted the fix/navigator-is-initial branch September 24, 2024 09:43
@github-actions github-actions bot added this to the Gutenberg 19.4 milestone Sep 24, 2024
Copy link

There was a conflict while trying to cherry-pick the commit to the wp/6.7 branch. Please resolve the conflict manually and create a PR to the wp/6.7 branch.

PRs to wp/6.7 are similar to PRs to trunk, but you should base your PR on the wp/6.7 branch instead of trunk.

# Checkout the wp/6.7 branch instead of trunk.
git checkout wp/6.7
# Create a new branch for your PR.
git checkout -b my-branch
# Cherry-pick the commit.
git cherry-pick efd622d95ebd6721c844949d57c559de67b17101
# Check which files have conflicts.
git status
# Resolve the conflict...
# Add the resolved files to the staging area.
git status
git add .
git cherry-pick --continue
# Push the branch to the repository
git push origin my-branch
# Create a PR and set the base to the wp/6.7 branch.
# See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request.

@ciampo
Copy link
Contributor Author

ciampo commented Sep 24, 2024

The backport has a conflict because of #65564 not being backported yet. Let's wait for that PR to be backported first

@ciampo ciampo added Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Sep 24, 2024
@github-actions github-actions bot removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 24, 2024
gutenbergplugin pushed a commit that referenced this pull request Sep 24, 2024
* Navigator: fix isInitial logic

* CHANGELOG

---

Co-authored-by: ciampo <[email protected]>
Co-authored-by: tyxla <[email protected]>
@github-actions github-actions bot added the Backported to WP Core Pull request that has been successfully merged into WP Core label Sep 24, 2024
Copy link

I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: bc3af46

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants