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

Reset banner bug fix #18

Merged

Conversation

lauslim12
Copy link
Contributor

@lauslim12 lauslim12 commented Oct 23, 2020

Hello!

This PR fixes the bug as explained in issue #17, the changes are simple:

  • The cause of this bug is in the reset() function in the src/components/app.jsx file. Previously, it was made to reset the currentDetails value to beginners-wish in the React state variable (I believe this is the cause of the problem). I fixed the problem by removing the currentDetails and adding the selectedWish state variable whose value is the current banner that the user is browsing on when he/she clicked the reset button.
  • Next, I adjusted the else statement in the src/components/banners.jsx to not set the selectedBanner state variable to beginners-wish. That way, when we clicked the reset button, we will still remain in our currently selected banner, not scrolling back to the Beginner's Wish.
  • I haven't updated the version number yet, just in case.

I have tested it in my web server (this includes last PR (#16) changes), and it works like a charm! No more switching to the beginner's wish if we rolled after clicking the reset button!

Don't forget to re-run npm run build after merging my previous and this PR, because I made this PR based on your master branch, not my cache-busting-improvements branch 😅.

I think that's all. Hope it helps and thank you!

Resolves #17

@uzair-ashraf
Copy link
Owner

Hey dude! Sorry I haven't had time to merge your last pr, I have to go out of town today so I'll take a look when I have a stable internet connection.

@lauslim12
Copy link
Contributor Author

Sure thing man! Take your time and don't worry about it haha!

Copy link
Owner

@uzair-ashraf uzair-ashraf left a comment

Choose a reason for hiding this comment

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

LGTM

@uzair-ashraf uzair-ashraf merged commit a91d0c2 into uzair-ashraf:master Oct 24, 2020
@lauslim12 lauslim12 deleted the reset-banner-bug-fix branch October 24, 2020 16:54
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.

Banner changed to Beginner Wish after reset and pulling again on the same banner
2 participants