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

Add warning not to use browser back button #619

Closed
1 task
Tracked by #59
alyssadai opened this issue Nov 8, 2023 · 3 comments · Fixed by #650
Closed
1 task
Tracked by #59

Add warning not to use browser back button #619

alyssadai opened this issue Nov 8, 2023 · 3 comments · Fixed by #650
Assignees
Labels
documentation Changes only affect the documentation quick fix Minimal planning and/or implementation work required. usability Issue affecting user or developer experience.

Comments

@alyssadai
Copy link
Contributor

alyssadai commented Nov 8, 2023

There should be a warning on the bottom of the page not to click the "back" button on the browser (which seems to cause some elements to persist weirdly - not sure if this is expected or a bug) but use the navigator at the top of the app instead.

I'm pretty sure the reason this breaks is that we're manually setting the "currentPage" in the state, when instead we should just implement a getter. I.e.

No good, make go away:

currentPage: "home",

and
setCurrentPage(p_state, p_pageName) {
p_state.currentPage = p_pageName;
},

Instead

this.$route.name

should either be used directly instead of the store access or be made into a getter and then used everywhere.

  • Find a way that this doesn't break
@alyssadai alyssadai added the documentation Changes only affect the documentation label Nov 8, 2023
@surchs surchs added this to Neurobagel Nov 8, 2023
@alyssadai alyssadai added usability Issue affecting user or developer experience. type:bug labels Nov 9, 2023
@surchs surchs moved this to Backlog in Neurobagel Nov 13, 2023
@surchs surchs added the quick fix Minimal planning and/or implementation work required. label Nov 27, 2023
@surchs surchs moved this from Backlog to Specify - Done in Neurobagel Nov 29, 2023
@surchs surchs moved this from Specify - Done to Implement - Active in Neurobagel Nov 29, 2023
@surchs surchs self-assigned this Nov 29, 2023
@surchs
Copy link
Contributor

surchs commented Nov 29, 2023

Addressing together with #491

@surchs
Copy link
Contributor

surchs commented Nov 29, 2023

Turns out this.$route.name is not available in the store. So for now I will add this as a computed property to every component that needs it. Not great, would be nicer to have it in the store. But putting it there is a bit involved: https://stackoverflow.com/questions/65995093/vue-js-get-current-route-in-vuex-module

@surchs
Copy link
Contributor

surchs commented Nov 29, 2023

What I think needs to happen:

  • every component gets a computed property: currentPage that returns this.$route.name
  • in the store
    • state
      • currentPage removed
      • pageData -> rename key for home to index (the actual route name
    • mutations
      • setCurrentPage removed
    • getters
      • getNextPage -> needs to accept an argument of currentPage and return the page like that

This is quite involved. At this point making pushing the route into the store from the router does not seem like such a bad idea anymore ...

@surchs surchs moved this from Implement - Active to Implement - Done in Neurobagel Nov 29, 2023
@rmanaem rmanaem moved this from Implement - Done to Review - Active in Neurobagel Nov 29, 2023
@github-project-automation github-project-automation bot moved this from Review - Active to Review - Done in Neurobagel Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Changes only affect the documentation quick fix Minimal planning and/or implementation work required. usability Issue affecting user or developer experience.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants