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

gatsby-link: add push (same as navigateTo) and replace methods #5934

Merged
merged 6 commits into from
Jun 29, 2018
Merged

gatsby-link: add push (same as navigateTo) and replace methods #5934

merged 6 commits into from
Jun 29, 2018

Conversation

tremby
Copy link
Contributor

@tremby tremby commented Jun 15, 2018

This addresses #5910.

This patch adds a second argument to gatsby-link's navigateTo, which will accept an object. If the object has a truthy replace member, history.replace is used rather than history.push. See the linked issue for my use case.

Since following the history API's replace and push methods (as suggested in the issue by @KyleAMathews) would be a breaking change, perhaps this is a good solution in the mean time for the 1.x line?

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 15, 2018

Deploy preview for using-drupal ready!

Built with commit 8a8d892

https://deploy-preview-5934--using-drupal.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Jun 15, 2018

Deploy preview for gatsbygram ready!

Built with commit 8a8d892

https://deploy-preview-5934--gatsbygram.netlify.com

@KyleAMathews
Copy link
Contributor

Let's just add replace and push methods that do the same thing as navigateTo and keep navigateTo around for backwards compatibility.

@KyleAMathews
Copy link
Contributor

Oh, guess we can keep this open as it'd be pretty easy to modify this PR :-)

@tremby
Copy link
Contributor Author

tremby commented Jun 16, 2018

Should I then modify the documentation and examples to use push?

@KyleAMathews
Copy link
Contributor

Hmmm no, let's add it in v1 and document it there but make the change and deprecate it in v2 then remove navigateTo in v3. So would you mind making another PR to v2 to add a dev-only deprecation warning that navigateTo is going away? To make this a bit more confusing, we're merging v2 into master tomorrow :-) so It might actually be that you need to land this first on v2 (now master) and then back port this to v1.

Anyways, I can help you through that.

@tremby
Copy link
Contributor Author

tremby commented Jun 16, 2018

OK, while I intend to also deal with v2, it looked today like it has not yet been merged, and so I have amended this v1 patch to have push and replace methods. Let me know if you want any changes. I'll soon get to v2.

@tremby tremby changed the title navigateTo: accept options in 2nd arg, add replace mode gatsby-link: add push (same as navigateTo) and replace methods Jun 16, 2018
@tremby
Copy link
Contributor Author

tremby commented Jun 16, 2018

I've tweaked this version a little since my last comment, and I've also made a patch for v2 (PR referenced above).

@tremby
Copy link
Contributor Author

tremby commented Jun 17, 2018

Fixed a broken test and replaced the commit.

@tremby
Copy link
Contributor Author

tremby commented Jun 18, 2018

The errors in the CI build failure don't seem connected to this patch; they're about componentDidReceiveProps being deprecated. Is there anything I need to do?

KyleAMathews
KyleAMathews previously approved these changes Jun 21, 2018
@KyleAMathews
Copy link
Contributor

Oh, there's a lot of conflicts actually — could you resolve those?

@pieh
Copy link
Contributor

pieh commented Jun 25, 2018

Are we adding this to v1?

@tremby
Copy link
Contributor Author

tremby commented Jun 25, 2018

Oh, there's a lot of conflicts actually — could you resolve those?

I can most likely do this later today.

Are we adding this to v1?

That was the idea; see this earlier comment #5934 (comment)

@tremby
Copy link
Contributor Author

tremby commented Jun 27, 2018

Rebased on the v1 branch. I'm not sure how to change the base of this pull request to v1... I can either close this and open a new PR, or you can manually merge with v1 your end:

git checkout v1
git fetch git@github.com:tremby/gatsby.git navigate-to-options
git merge FETCH_HEAD
git push

@tremby
Copy link
Contributor Author

tremby commented Jun 27, 2018

Oh, in the rebase I lost your commit 999f1baa9 and I can't seem to fetch it from Github or follow the link to it above in this thread. Oops...

@tremby
Copy link
Contributor Author

tremby commented Jun 27, 2018

Haha, but I can click the link to it from the comment I just posted. Github bug, methinks. Will add it...

@tremby tremby changed the base branch from master to v1 June 27, 2018 01:18
@tremby
Copy link
Contributor Author

tremby commented Jun 27, 2018

Added your readme change commit back on, and found the option to change the base for this PR. Should be ready to go.

Copy link
Contributor

@m-allanson m-allanson left a comment

Choose a reason for hiding this comment

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

Thanks @tremby!

I updated the readme example and added a small note about push previously being named navigateTo

@tremby
Copy link
Contributor Author

tremby commented Jun 28, 2018

@m-allanson, I actually did have something like that but it was taken out in Kyle's commit: 8a8d892

Doesn't matter to me either way though! Thanks.

@tremby
Copy link
Contributor Author

tremby commented Jun 28, 2018

...Did you mean to merge this into v1 but accidentally merge v1 into this instead?

@m-allanson
Copy link
Contributor

Ah sorry for the delay, GitHub wanted me to update the branch before merging. Merging now.

@m-allanson m-allanson merged commit f5b2c56 into gatsbyjs:v1 Jun 29, 2018
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.

5 participants