-
Notifications
You must be signed in to change notification settings - Fork 7
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
Next page accessibility/navbar/layout refactor + All page refactor #233
Next page accessibility/navbar/layout refactor + All page refactor #233
Conversation
@surchs I believe this is ready for review actually. Possible I just hadn't converted it back from draft after the last commit. |
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 for the PR @jarmoza. Very exciting to see how much slimmer the codebase is getting here! 🎉 I have reviewed this mostly as the two updated nav-bar components (next-page
and tool-navbar
) and have left some comments. Take a look, but I think this is good to go with the context that we're going to finish the whole refactoring with a major store overhaul.
<script> | ||
|
||
// Allows for reference to store data by creating simple, implicit getters | ||
import { mapGetters } 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.
You could handle all three map...
imports here inside the curly braces (e.g. import { mapGetters, mapMutations, ... }
). Is there a reason you prefer to have them each on one line?
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.
Yes, agreed. This is leftover from when we had wanted to explicitly state what the map
imports were for. So I had just taken to copy pasting the single line with the comment. At this point, this import list can be extended throughout the codebase sans comments.
<b-col class="text-right" cols="4"> | ||
|
||
<!-- Optional instructions to remind user of criteria to proceed to the next page --> | ||
<p class="instructions-text" v-if="!pageAccessible(currentPage)"> |
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.
What do you think about using the vue-bootstrap tooltip component here: https://bootstrap-vue.org/docs/components/tooltip ?
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 we're better off being visually explicit about the steps needed to gain next page access. The tooltip is visually cleaner but it's not an obvious UI affordance - it would turn into something the user accidentally discovered.
@@ -42,13 +43,16 @@ | |||
|
|||
<script> | |||
|
|||
export default { | |||
// Allows for reference to store data by creating simple, implicit getters | |||
import { mapGetters } 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.
Same question as with next-page.vue
: could you import them all in one line? I also think the comments explaining how we use vuex may be better placed in a README or in our self- or dev-facing docs.
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.
Yes. See my comment.
<p class="instructions-text"> | ||
{{ uiText.categorySelectInstructions }} | ||
</p> | ||
</b-row> |
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 can't mount this and check directly, but wouldn't that repeat the issue with the category-select-table being shifted downward compared to the column-linking-table here? I thought the goal was for both components to be aligned with their upper edge here.
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.
Yes, but given the likely discrepant heights of the tables (column linking will usually be larger, I think it's better to slightly center the category select component.
To more directly answer your question though, the reason this change is made here is that the categorization page now acts more like a layout, removing that functionality from the component itself.
:title="uiText.categorySelectTitle" | ||
@category-select="setSelectedCategory($event)" /> | ||
:selected-category="selectedCategory" | ||
@category-select="setSelectedCategory($event.category)" /> |
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.
just making a note here that I'm ignoring the events and props because I expect them to be replaced in an upcoming refactor of the store.
@@ -9,6 +9,8 @@ export const state = () => ({ | |||
|
|||
currentPage: "home", | |||
|
|||
columns: [], | |||
|
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.
just another note here that I'm not going through this in detail because I expect the store to be fundamentally re-implemented following our component refactoring.
This PR represents changes that we have planned for I/O refactor for the annotation tool's pages but also our refactor next page accessibility/the navbar and default layout.
Since most of these changes are interlinked, it made sense to include them in one PR. This is also a "in the dark" PR given that we have no good means of testing the following changes throughout the app because of the ongoing refactor. This ties together issues #232, #218, #214, #215, #216, and #217.
So the code review for this will be more of a visual inspection of the changes made. (I will leave it as a draft for now as @surchs may continue this work next week while I am away.)
Below is a summary of the changes made thus far.
Next page and Layout
This is a new component that sits on the bottom right of the layout. It is chiefly a next page button whose state is reactive to that of the app's (see new function
nextPageAccessible
in the store).The layout has been slimmed down considerably as well given our I/O refactor specifications.
Tool navbar
I/O refactor changes have been made in addition to incorporation of new page accessibility/store functionality.
Store
mapState
syntax.pageAccessibility
andnextPageAccessibility
.All pages
map*
syntax.columnToCategoryMap
, this has been moved to the index page and there is no longer anyinitializePage
calls. Each page'smounted
call just sets thecurrentPage
. As such, all tool navigation is chiefly reactive viapageAccessibility
(in the tool bar and next page button components).e2e test files
Adjustments for above store/refactor changes have been made.