-
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
Assessment tool group tabs and enhanced functionality #150
Conversation
Default layout has now been given a name. This is used in a new vue/eslint rule that ignores the fact that default.vue is a single word component. Default.vue is used by Nuxt to indicate the default layout for the app.
1. Tool groups are now in the store as `toolGroups` 2. Cleaned up instruction text for tool grouping 3. Ensured tool group table is repopulated when navigating back to categorization page 4. The assessment tool tab on the annotation page has been replaced with tabs for tool groups 5. Tab width for annotation tabs has been set to `col-2` 6. The criteria to move on to the annotation page from the categorization page has been expanded to include whether or not all columns marked as assessment tools have been grouped together. See `nextPageAccessible()`
Tool groups are now properly added/deleted using Vue.set/Vue.delete. And tool groups are removed from the annotation details list when they are deleted on the categ-tool-group component
Hi @jarmoza. Thank you for the PR. This looks like a lot of work. Could you please do either: so that we we can look at this together with the other recent additions and fixes (and also resolve these pesky merge conflict messages). |
components/annot-tab.vue
Outdated
@@ -82,6 +82,12 @@ | |||
const columnList = []; | |||
for ( const columnName in this.columnToCategoryMap ) { | |||
|
|||
// If this tab is an assessment tool group, make sure this column is in its toolset | |||
if ( Object.prototype.hasOwnProperty.call(this.details, "groupName") && |
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.
The Object.prototype...
thing made me look twice. If I understand right, ESlint flags this because the default this.details.hasOwnProperty
could fail if this.details
happens to have a property called "hasOwnProperty"
? The recommended way to work around this (if legacy browser support isn't a barrier) seems to be to use hasOwn
instead, which is the replacement of hasOwnProperty
.
I'd suggest we use hasOwn
here or if not add a comment why were are using this less expected form.
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.
Yup. That's right. Object.prototype.hasOwnProperty.call
is the suggestion to make sure of the correct behavior. hasOwn
is newer, considers a few more edge cases, and "is more intuitive." Sounds right.
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, this is working and looks very nice!
It also solves our need to group columns together so we can annotate them together and then define missing values.
That said, I think in a future PR we should redesign the tool-grouping workflow. The main issue is that we now have an additional thing in the store to keep aligned with the columnToCategoryMapper we already have, and this alignment is hard to do, we need watchers, and refresh methods and other things that are easy to break. See for example the bug I described where, if you assign two columns to a tool, and then remove one of the columns via the annotation window, the column is still assigned to the tool, but no longer designated as a "assessment tool" category.
I think at the core, the "map columns to tool groups" workflow is the same as the "map columns to categories" workflow. The only difference is that the category you are assigning columns to (i.e. the tool group) has to be created first. That is, if we find a way of creating new categories for the assessment tool groups, then we don't have to have separate store variables and UI elements to keep columns and tool groups aligned with columns and categories.
That said, the functionality right now is all we really need to start using the tool (and to add the assessment tool group annotation functionality) and so I suggest we merge this PR with some minor changes (as commented) and think about my above points in a separate issue.
// Remove this value from the column's missing value list in the store | ||
|
||
// 1. Remove the item from table data | ||
this.tableItems = this.tableItems.filter(item => |
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.
Is this change related to the annotation tabs? If so, I don't think I understand what it does. Otherwise I'd say we add that to the missing values PR and discuss it there.
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 am too a bit puzzled by this line. I don't quite recall writing it. Just looking at what the function is supposed to be doing though, perhaps an answer to this question would clear things up.
When a value is removed from the missing value table how else is tableItems
being updated? I don't see any other place it's touched other than in created()
.
variant="info" | ||
@click="createToolGroup()"> | ||
{{ uiText.createToolGroupButton }} | ||
@click="( modes.create === currentMode ) ? createToolGroup() : modifyToolGroup()"> |
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.
neat!
|
||
// 1. Check to see if any columns have been unlinked as assessment tools | ||
const toBeRemoved = []; | ||
for ( const item of this.assessmentToolGroups.items ) { |
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.
There is something weird going on here when the columnToCategoryMap
have been changed (e.g. by removing a column from a tool group in the annotation window). The problem seems to be that although this.assessmentToolGroups
clearly has a property items
with a non-empty array, directly accessing this array via this.assessmentToolGroups.items
(e.g. console.log(this.assessmentToolGroups.items)
shows an empty array.
Take a look here:
If you access the variable directly or watch it via vue-dev tools, all looks normal. But obviously you cannot iterate over an empty array so you never get to check if anything has to be removed here.
As I mention in the review, i think the real solution to this is to redesign the way the tool groups are implemented. But we should do this in a later PR, right now we get what we need and this bug is something we can work around.
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, to redesign. One of the reasons I had wanted to take a look at a potential object-oriented reworking is because things are beginning to feel to disparate/unwieldy in the store and in turn too constrictive (a.k.a. 'brittle'). Too many sources of truth. That said, that's not for now. So let's discuss this smaller internals redesign next.
- I'm going to take a look into this for a little bit so I can better understand what is happening.
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.
So, from what I can tell there is some disconnect in how you're viewing the object via the browser (client) console. Debug messages from the server as well as Vue dev-tools reveal assessmentToolGroups.items
to be correctly populated.
That said, I have produced a fix for the issue of the table not being updated and though we should readdress how this is done in the redesign we mentioned, we have some nice "magic" at hand.
In the annotation pages unlinkColumnFromCategory
method - which is called when a column is removed from the annot-column
component, I have added the following third step to appropriately update the tool groups in the store.
// 3. If this column was a grouped tool, remove it from its group
if ( this.isToolGrouped(p_event.removedColumn) ) {
// A. Remove the tool from its group
const groupName = this.getGroupOfTool(p_event.removedColumn);
this.$store.dispatch("removeToolFromGroup", {
group: groupName,
tool: p_event.removedColumn
});
// B. If its former group is now empty of tools, remove the group itself
for ( const groupName in this.toolGroups ) {
if ( 0 === this.toolGroups[groupName].length ) {
this.$store.dispatch("removeToolGroup", { name: groupName });
}
}
}
This duplicates behavior from the categ-tool-group
component itself, but applies the changes we want reflected on the categorization page.
And here's the Vue "magic" part. If every tool is removed from its tab on the annotation page (annot-columns
component), the tab itself is now removed.
This PR addresses functionality listed in both #118 and #138. See each issue for the desired functionality checklists.