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

Issue 2977 #3798

Closed
wants to merge 4 commits into from
Closed

Issue 2977 #3798

wants to merge 4 commits into from

Conversation

vrrayz
Copy link
Contributor

@vrrayz vrrayz commented Nov 3, 2022

Back button on openings now list to list page as requested from this this issue

@vercel
Copy link

vercel bot commented Nov 3, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
dao ✅ Ready (Inspect) Visit Preview Dec 13, 2022 at 1:22AM (UTC)
pioneer-2 ✅ Ready (Inspect) Visit Preview Dec 13, 2022 at 1:22AM (UTC)
pioneer-2-storybook ✅ Ready (Inspect) Visit Preview Dec 13, 2022 at 1:22AM (UTC)

Copy link
Collaborator

@traumschule traumschule left a comment

Choose a reason for hiding this comment

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

LGTM! Keep on it with #3768 if you want.

@traumschule traumschule added community-dev issue suitable for community-dev pipeline jsg-code-review labels Nov 3, 2022
@traumschule traumschule requested a review from WRadoslaw November 3, 2022 13:53
@vrrayz vrrayz mentioned this pull request Nov 3, 2022
1 task
Copy link
Contributor

@WRadoslaw WRadoslaw left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@WRadoslaw WRadoslaw left a comment

Choose a reason for hiding this comment

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

Actually, I just noticed that this solution causes one bug. Problem is that when you go into opening details from a specific WG page, the back button will always transfer you to general openings instead of given WG ones.

Two cases have to be considered before pushing customLink to the stack:

  • Whether history.length is bigger than 2 (if stack eq 1 then, the user clicks the link and get redirected, if stack eq 2 there is a possibility that the user opened a new tab in the browser and paste in the link
  • Another possibility is that the user clicks on the URL to the openings list and then clicks on the given opening, in that case, checking for stack length will fail (stack length will equal 2, but goBack should be invoked anyway), but we can check the current action on the history. I see that when you paste the URL or click on the link action will eq POP while if you navigate from Pioneer forward (Opening list -> Opening details) it will be PUSH

Integrating these two rules should do the work. WDYT?

@vrrayz
Copy link
Contributor Author

vrrayz commented Dec 13, 2022

Actually, I just noticed that this solution causes one bug. Problem is that when you go into opening details from a specific WG page, the back button will always transfer you to general openings instead of given WG ones.

Two cases have to be considered before pushing customLink to the stack:

  • Whether history.length is bigger than 2 (if stack eq 1 then, the user clicks the link and get redirected, if stack eq 2 there is a possibility that the user opened a new tab in the browser and paste in the link
  • Another possibility is that the user clicks on the URL to the openings list and then clicks on the given opening, in that case, checking for stack length will fail (stack length will equal 2, but goBack should be invoked anyway), but we can check the current action on the history. I see that when you paste the URL or click on the link action will eq POP while if you navigate from Pioneer forward (Opening list -> Opening details) it will be PUSH

Integrating these two rules should do the work. WDYT?

I just made modifications based on your second suggestion. I think tracking the link action is all that's needed. I tested out for those cases you mentioned, and they work fine now

Copy link
Collaborator

@traumschule traumschule left a comment

Choose a reason for hiding this comment

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

lgtm! tested entering opening details from /working-groups/openings and /working-groups/forum.

Copy link
Contributor

@oleksanderkorn oleksanderkorn left a comment

Choose a reason for hiding this comment

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

Why does it have all the types changes here? I guess this can be done in a separate change if needed.

@traumschule
Copy link
Collaborator

traumschule commented Dec 14, 2022

You are right, didn't look at the code. @vrrayz resubmit the fix on a new branch please.

@vrrayz
Copy link
Contributor Author

vrrayz commented Dec 14, 2022

You are right, didn't look at the code. @vrrayz resubmit the fix on a new branch please.

alright

@vrrayz
Copy link
Contributor Author

vrrayz commented Dec 14, 2022

@traumschule I just resubmitted in this branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builders-wg-code-review community-dev issue suitable for community-dev pipeline
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants