-
Notifications
You must be signed in to change notification settings - Fork 6
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
Navbar/next page button/page accessibility update for refactor #328
Conversation
Hey @jarmoza, thanks for the PR. I think I'm either missing a piece of info or I disagree here. I'll quickly reply on your explanation.
Agree. My understanding is that the
Agree about vue-computed reactivity in general (as in your link). But the Let me know if there is something else I'm missing about the getter, but if not, I would say let's use the getter we already have here and avoid adding the extra actions and watchers. |
@surchs I think the short answer to this is that while I understand what you're saying the reactivity for |
@surchs So, in summary. Your code "works" for the nav as does the current code in the dev branch. In fact, we can currently navigate between the pages using the appropriate nav item even though it appears disabled (gray). Hovering over the activated nav item reveals a finger pointer which allows a click and pass through to the next page. There was just one more step to make it fully work – and that's the line you have added in your patch above. With the check to So agreed with your changes in the patch. We no longer need the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me @jarmoza. There is a mapActions
left in the tool-navbar
that can be removed. With that good to go 🎉!
components/tool-navbar.vue
Outdated
@@ -52,6 +52,9 @@ | |||
// Fields listed in mapState below can be found in the store (index.js) | |||
import { mapState } from "vuex"; | |||
|
|||
// Allows for reference to store actions (index.js) | |||
import { mapActions } from "vuex"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be removed now as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @surchs. Will remove and merge.
Closes #315 and #317
Some agent in the tool must be responsible for indicating that a page is accessible to the user (on the page via user action, or at some higher level like the navbar or layout, etc.) Previously, actions on a page by a user would manually change the accessibility status of the next page (via an action call to change
pageData.<pageName>.accessible
).The navbar now keeps a watch on the store for changes, updating the accessible status of pages via
updatePageDataAccessibility
as the relevant changes are made to the store that would unlock them. This has the side effect of also keeping the next page button status on each page up to date with the state of the store.The more complex Vuex getter
isPageAccessible
could not be reactive because it currently returns a boolean. Vue recalculates computed values based on whether or not values used in the computed function to create the return value have changed.. In the case of a primitive like a boolean with one return statement,isPageAccessible
would only be calculated once. However, page accessibility state depends on multiple variables in the store depending on which page is being checked as accessible. Even a re-implementation of the function with multiple return statements does not produce the desired reactivity.There are at least two methods for making sure that store changes trigger a check for page accessibility. The agent (in this case the
tool-navbar
component) can either place a watch on the specific store variables that may be involved in a state change OR it may deeply watch$store.state
itself. (Suggestion for solution found here.)This is accomplished via the placement of a watch in the
created
hook:The latter is the one implemented with this PR. A change in
$store.state
triggers a call to new store actionupdatePageDataAccessibility
. Thedisabled
attribute in the navbar and next page button are then simply tied to thepageData.<pageName>.accessible
field which will always reflect the current state of the store.