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

Fixed browser Back Button anomaly on PDP #3032

Merged
merged 3 commits into from
Oct 6, 2017

Conversation

aanchirinah
Copy link
Contributor

Resolves #2660 ,

So there was a Router.go snippet in varaintList.js that was changing the URL when a variant is selected, so based on @mikemurray comment on how to fix the issue , I refactored the composer object (in productDetail.js) to get variantId by using ReactionProduct.selectedVariant()._id rather than fetching it as a param from the URL.

HOW TO TEST

  • Go to "Basic Reaction Product"
  • Click on Red and add to cart
  • Click on Green and add to cart
  • Click on Back Button of your browser and you will notice it actually goes back as it should do.

@@ -272,7 +272,7 @@ function composer(props, onData) {
const tagSub = Meteor.subscribe("Tags");
const shopIdOrSlug = Reaction.Router.getParam("shopSlug");
const productId = Reaction.Router.getParam("handle");
const variantId = Reaction.Router.getParam("variantId");
const variantId = ReactionProduct.selectedVariant()._id;
Copy link
Member

Choose a reason for hiding this comment

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

You could probably use ReactonProduct.selectedVariantId() instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I just refactored it to selectedVariantId()

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

This works and fixes the problem but I want to understand why the change to branchAsset is in here. Is it part of this fix?

@@ -19,7 +19,7 @@ function composer(props, onData) {
}

if (shop && Array.isArray(shop.brandAssets)) {
const brandAsset = _.find(this.getShop().brandAssets, (asset) => asset.type === "navbarBrandImage");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason this change was added to this PR? If this is fixing an another issue it should be explicitly called out in the comments. Generally we should keep bugfix PR's pretty focused on fixing just the one issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got that change in brandAssets when I pulled recent changes from marketplace branch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

how would that be possible? If you are up to date with marketplace then why would you have that change? Did you merge a conflict?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a typographical error in my commit message so I reset my HEAD and unknowingly unstaged the change that was made to the brandAssets.

@@ -19,7 +19,7 @@ function composer(props, onData) {
}

if (shop && Array.isArray(shop.brandAssets)) {
const brandAsset = _.find(this.getShop().brandAssets, (asset) => asset.type === "navbarBrandImage");
const brandAsset = _.find(shop.brandAssets, (asset) => asset.type === "navbarBrandImage");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is identical to the line in the current marketplace branch so I'm confused as to why it's showing this as a change, but it appears to be correct.
https://github.com/reactioncommerce/reaction/blob/marketplace/imports/plugins/core/ui-navbar/client/containers/navbar.js#L22

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled the latest changes from marketplace , I made a typographical error in my commit message so I reset my HEAD and unknowingly unstaged the change that was made to the brandAssets.

@spencern spencern changed the base branch from marketplace to release-1.5.0 October 6, 2017 15:43
@spencern spencern self-requested a review October 6, 2017 15:44
Copy link
Contributor

@spencern spencern left a comment

Choose a reason for hiding this comment

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

Approving the code as the change to navbar.js line 22 appears to be identical to the current marketplace branch.

@spencern spencern merged commit ee670d0 into release-1.5.0 Oct 6, 2017
This was referenced Oct 6, 2017
@spencern spencern deleted the audax-fix-issue-2660 branch November 9, 2017 06:45
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.

4 participants