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
bug:ux Unexpected and unintended behavior that is detrimental to the user experience. documentation Changes only affect the documentation quick fix Minimal planning and/or implementation work required. type:bug Defects in shipped code and fixes for those defects

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
@alyssadai alyssadai added bug:ux Unexpected and unintended behavior that is detrimental to the user experience. type:bug Defects in shipped code and fixes for those defects labels Nov 9, 2023
@surchs surchs added the quick fix Minimal planning and/or implementation work required. label Nov 27, 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 ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:ux Unexpected and unintended behavior that is detrimental to the user experience. documentation Changes only affect the documentation quick fix Minimal planning and/or implementation work required. type:bug Defects in shipped code and fixes for those defects
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants