-
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
[FEAT] display categorized columns on the annotation page #77
Conversation
- replaced 'annot' prefix with 'category'
- added functionality to display columns currently assigned to a category - linked the new component to the category subpages - hardcoded a list of categorized columns on the annotation page. this should be replaced by the appropriate global store variable
- new component annot-age-values.vue to show raw and transformed age - new component annot-continuous-values.vue to display continuous values in a table. May include ability to suggest changes to transformed value in the future Resolves #59
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.
Okay. So overall, functionally, the pieces in this PR look good. I didn't notice any errors. Obviously, there are pieces that will be fleshed out more in the future, bits that need to be hooked to the store, and aesthetic changes as discussed. So most of my comments below and specific to the files have to do with config and coding practices.
And nicely done for your first complex piece of Vue code!
General comments:
-
Now that components are getting more complicated, we should use nested directories within the components directory. Though initially this would require the component name to include a concatenated version of the subdirectory path in its name, this can be overcome by placing the path in
components.dirs
innuxt.config.js
. See: https://nuxtjs.org/docs/directory-structure/components/#nested-directories -
One of the reasons I heavily comment code is through lessons learned from past development and collaborative projects where either I forgot what code was doing or people needed to quickly know without parsing my code what it was intended to do. Overall, even though it does not change the functionality I would suggest taking some time now while the code is relatively fresh in your mind to go through all of these new files in this commit and provide some comments - particularly for the functions that are producing values used by and shared between components in the interface.
-
Complex return values (see examples
filteredTable
,uniqueValues
,unique_table_data
,applyTransform
inannot-age-values.vue
andfilteredColumns
inannot-columns.vue
):- Though it's tempting to use this kind of syntax to produce a return value in one line, code like this becomes confusing for people who are approaching it. I would recommend breaking down this return value creation process into several lines before the final value is returned.
- As I mention in one comment in
annot-columns.vue
, maybe we should institute some rule for the number of levels deep a (return) statement can be before it should be broken up into several distinct steps in our code.
-
Given our discussion re: semicolons, this would be a good chance to insert them in this PR.
-
Consider blank lines between distinct pieces/steps of functionality within a method. They can make code more readable, if a little less compact.
return `${yearValue + monthValue}` | ||
} | ||
}, | ||
applyTransform() { |
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.
See my general comment on complex return values
) | ||
) | ||
}, | ||
unique_table_data() { |
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.
See my general comment on complex return values
} | ||
) | ||
}, | ||
uniqueValues() { |
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.
See my general comment on complex return values
.filter(([columnName, categoryName]) => categoryName === this.activeCategory) | ||
.map(element => element[0]) // return only the column name that was assigned to this.activeCategory | ||
}, | ||
filteredTable() { |
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.
See my general comment on complex return values
<div> | ||
<b-card no-body> | ||
<b-card-header>I am a header</b-card-header> | ||
<b-card-body style="position:relative; height:300px; overflow-y:scroll;"> |
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.
Move to style tag below for this class or create an id for this element to reference in CSS
return { | ||
// TODO turn the range into an argument for the component | ||
unique_range: {start: 0, end: 3}, | ||
regularExpressions: [ // using named capture groups |
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.
It's best to heavily document regular expressions when they are created. This way when you or someone new revisits the regex code, all pieces of the expression are immediately available without visual parsing of them.
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! And agreed, I had meant to add detailed comments here. Will add
<b-card no-body> | ||
<!-- TODO: make this also toggleable like the explanation tab--> | ||
<b-card-header>Review the annotated columns</b-card-header> | ||
<b-card-body style="height:30vh; overflow-y:scroll;"> |
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.
Move to style tag below for this class or create an id for this element to reference in CSS
<b-list-group> | ||
<b-list-group-item | ||
class="d-flex justify-content-between align-items-center" | ||
v-for="[columName, categoryName] of Object.entries(filteredColumns)"> |
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.
Why is Object.entries performed twice 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.
I'm not sure I understand what you're referring to here? I use Object.entries
to iterate through the object. afaik that's like dict.items()
in python. filteredColumns
is constructed from an array of two-tuples (key, value) using Object.fromEntries
. Is that what you are referring to?
// "columns" is an object of the form | ||
// { columnName: "CategoryName", ... } that contains all annotated columns and the categories they were mapped to | ||
// In normal use, this object is kept in the global state and is passed here via the parent components | ||
return Object.fromEntries( |
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.
This is compact, but can you please write this in a few steps? And with comments on the process?
(Maybe we should put a limit on the levels deep of complexity a return value can have before we write the steps of the code separately.)
data() { | ||
return { | ||
explanation: 'This is an explanation for how to annotate age', | ||
activeCategoryName: 'Age' |
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 presume this is a store value? (Likewise across the other category-*
components.)
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 thought we keep this inside the component - just like the explanation. Are you thinking of some future where the "Age" category might have a different name?
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 noticed a warning coming from annot-columns.vue
, around line 10.
<b-list-group> | ||
<b-list-group-item | ||
class="d-flex justify-content-between align-items-center" | ||
v-for="[columName, categoryName] of Object.entries(filteredColumns)"> |
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'm getting the following warning from here:
WARN in ./components/annot-columns.vue?vue&type=template&id=6b92ec6b& friendly-errors 10:51:55
Module Warning (from ./node_modules/vue-loader/lib/loaders/templateLoader.js): friendly-errors 10:51:55
(Emitted value instead of an instance of Error) : component lists rendered with v-for should have explicit keys. See https://vuejs.org/guide/list.html#key for more info.
Thanks @jarmoza for the detailed review. That's very helpful!
I had used that first for the same reasons you mention. The reason I changed back to a flat list was that I read a convincing piece on best practice arguing against nested components. The bottom line was:
There are some other points in the piece, give it a read. What convinced me most was that it gets pretty annoying to navigate all these sub-dirs (how deep should you go) and then you don't know where a new component should go. So I'd vote for keeping things flat until we find that this suggestion doesn't work for us. agreed on the other points regarding commenting and more readable constructs for the returned values. As we discussed, I wonder if this means we should not use arrow functions for things that are more than one or two levels deep. |
- also editing the hardcoded variable in the annotation.vue page
OK, @jarmoza, as we had just discussed, I turned your unaddressed comments into new issues so we can address them after the demo but merge the functional code now to get ready for the demo. I'll merge this now as is. |
In this PR:
Resolves #57