-
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
Post demo cleanup - Annotation page component overhaul and formatting #108
Conversation
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 @jarmoza, this is a really excellent PR! The code got immensely more readable and clear. This is great, and great work!
I made some small comments, the main thing would probably be the eslint config to make sure we have consistent whitespace.
.eslintrc.js
Outdated
// "airbnb", // TODO: activate this styleset | ||
"eslint:recommended", | ||
"plugin:vue/strongly-recommended", | ||
// "plugin:prettier/recommended", // TODO: consider activating |
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'd say let's get rid of prettier and have eslint handle formatting when called.
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.
Sounds good to me at this point. I'll remove the commented out lines.
.eslintrc.js
Outdated
|
||
rules: { | ||
|
||
"no-unused-vars": [ "error", { |
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.
in our discussion we said we'll turn this into a warning instead so that unused variables are flagged.
components/annot-age-values.vue
Outdated
return [colName, value] | ||
export default { | ||
|
||
props: { |
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.
cool, I like the consistent ordering!
// (Currently just used for unused variables in v-for statements) | ||
"ignorePattern": "^_" | ||
}] | ||
} |
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.
My only comment here is that I don't think these rules enforce / fix indentation and trailing whitespaces. Apart from the actual code linting, this is one major improvement for us because we will avoid these messy git diffs that are mostly white-space differences.
So I would say let's make sure here that we get rid of trailing whitespace and that there is some opinionated rule about indentation - otherwise we'll continue having whitespace diffs and make our history hard to read.
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.
@surchs I've changed these to warnings and added some specific rules about indent, vue script-indent, comma spacing, and trailing commas.
The idea here is to explicit about the rules we want to enforce even though they might be default in eslint and vue-eslint rules?
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.
Cool. No, my comment was about the fact that when I run eslint with this config locally, I don't get yelled at for trailing whitespace and I can chose what level of indentation I want for each line as I see fit. I think we should pick some indentation style and remove trailing whitespaces so that its consistent in the code and we don't get diffs because one of us accidentally added or removed some whitespace.
overflow-y: scroll; | ||
} | ||
|
||
</style> |
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 excellent! Short, clear purpose, super legible!
|
||
for ( let index = 0; index < this.columnToCategoryTable.length; index++ ) { | ||
// A. Determine if category-column linking or unlinking has occurred | ||
const linking = !( this.selectedCategory === this.columnToCategoryMap[p_clickData.column] ); |
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.
Could this just be
const linking = !( this.selectedCategory === this.columnToCategoryMap[p_clickData.column] ); | |
const linking = this.selectedCategory !== this.columnToCategoryMap[p_clickData.column] ; |
? If so, I'd find that easier to read
store/index.js
Outdated
{ | ||
id: 1, | ||
category: "Sex", | ||
explanation: "This is an explanation for how to annotate sex.", |
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.
indent seems off
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.
Somehow tabs remain in this file. Fixed.
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.
Cool. But we should check why your eslint run didn't catch that.
store/index.js
Outdated
// 0. This annotation information is default but we can swap out and reinitialize | ||
// annotation data structures by calling 'initializeAnnotationDetails' with a new | ||
// object containing annotation information for each category | ||
const annotationDetails = [ |
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.
cool, I like this!
store/index.js
Outdated
}, | ||
|
||
// Tool navigation | ||
|
||
enablePage(p_context, p_navData) { |
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.
how about initializePage
? Maybe not quite the right name, but this method seems to do more than just make the link clickable, particularly in the future?
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.
Perfect. 👌
store/index.js
Outdated
|
||
p_context.commit("removeColumnCategorization", p_linkingData.column); | ||
}, | ||
|
||
// Annotation page actions | ||
|
||
revertColumnToOriginal(p_context, p_columnName) { |
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.
Could you add a docstring comment to explain what this method does and when it would be called? I assume this is when someone wants to reset an annotation?
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 you need to run one more eslint --fix
to get the remaining errors (annotation component and some js config files). Otherwise this is good to go 🎉 !
@surchs Yup! I noticed. Pushed the eslint changes for the pages in a separate commit outside the PR right afterward. |
General
uiText
object in the data of their respective componenttype
andrequired
. If a prop is given to a component but is not used, it is listed but not marked asrequired: true
.p_
hungarian notation to distinguish parameters from local variables. Some of this is just for consistency's sake for the time being until we introduce a prettifier - which will be coming up very soon.index.js
annotationDetails
field have been added to the store. This field contains information for generating the individual annotation tabs. This field holds the same purpose as the formerpages
field inannotation.vue
, but is now moved to the more central place of the store where it can be properly initialized via config (in the future). Each page object also contains a newoptions
field for any specialized parameters needed for any of the tabs. The field is initialized in thenuxtServerInit
action just like the categories.revertColumnToOriginal
. This is currently called when aremove
button is clicked in theannot-columns
component.columnDescription
andvalueDescription
.convertTsvLinesToDict
in the store has been renamed toconvertTsvLinesToTableData
in order to better reflect its purpose. But more importantly, a bug in this function was fixed that was still allowingblank lines in files to enter the data table.
category-*
category-*.vue
files have all been removed. They have been replaced with the more generalannot-tab
component.annot-tab
annot-tab
contains theannot-explanation
andannot-columns
components of the previouscategory-*
components but now includes acomponent
representing the data type specialized components for each category on the annotation page. The information to generate these tabs is now found in the store in theannotationDetails
field which is initialized in thenuxtServerInit
action.annot-tab
component:filteredTable
(nowfilteredDataTable
),relevantColumns
, anduniqueValues
annot-age-values
,annot-discrete-choices
, andannot-vocabulary
annot-tab
component that remain in the specialized annotation components have been standardized (i.e.saveButtonDisabled
,saveButtonColor
,applyAnnotation
,transformedValue
,initializeMapping
,updateMapping
, etc.)applyTransformation
has been renamed toapplyAnnotation
and a new helper function has been introduced for the sake of uniformity:transformedValue
. This latter function is the place where an annotation component can go to get the new value given a column and raw value. The hope was thatapplyAnnotation
could be raised up to theannot-tab
level but given some slight differences, it made sense to keep it here. In the very least,applyAnnotation
andtransformedValue
can give these components consistent function naming.initializeMapping
,updateMapping
, anduploadMapping
(future functionality withstanding).checkAnnotationState
functions just have the purpose of setting the disabled status of the save annotation button, that statement has been pulled out of the function. The disabled status is now more explicitly set via the new boolean return value ofcheckAnnotationState
instead.annot-columns
retrieveColumnDescription
has been implemented to display the data dictionary description of the column next to its name in this component. (Can always be dropped if the screen appears to be too cluttered because of it.) This uses the newcolumnDescription
retrieval method in the storev-for
key warning has been removed by adding the key ascolumnName
annot-vocabulary
getDescription
has been split into two functions and moved to the store since it contained general, reusable data dictionary functionality. These are the newcolumnDescription
andvalueDescription
functions