-
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
New global store version + new dynamic styling for categorization page #66
Conversation
…data and class names only for category linking on categorization page
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 for this large PR, this is a lot of work.
Having read over the code, I believe that we should make some changes that will allow us to handle this in a more compact form that is also more intuitive.
- Let's replace the
coloring-listgroup
component with a<b-table>
component.
We already know how to reactively style rows of a table and how to react to click-events there.
This will make it easier for us to split the "category-to-column-mapping" side from the "updating-the-UI-components-reactively" side. - And linked to 1.: let's use Vue for handling styling changes in response to changes of state and data, and let's avoid handling these things directly. We have discussed this before. We have adopted this framework to make things easier to understand, for us and future collaborators. But that means we also have to use the framework in the way it was intended/recommended, otherwise we don't get these benefits. From my review of the code, all instances where the DOM is currently directly access and edited can be handled more cleanly and compact via Vue.
To illustrate both points, I have coded up a small example here (its all in a single component though): https://codepen.io/surchs/pen/vYWqQKp?editors=1011
This uses @row-selected
to handle both the category-selection and the category-to-column mapping. And it uses :tbody-tr-class
to style the category-selection and category-to-column-mapper
elements reactively.
I think we should adopt a similar structure for the corresponding components here. It would probably make sense for us to discuss this in person. Let me know what you think.
components/coloring-listgroup.vue
Outdated
}, | ||
|
||
// Index of most recently clicked list group item | ||
clickedIndex: 0 |
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.
Let's use the actual category names here. We should have access to them from the global store. We can still initialize as null
. We can rename the variable to currentCategory
or something descriptive like that.
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.
Sure, this small change is possible though currently moot since clickedIndex
is only used as check to see if the same category is being clicked again. More on that in your next comment on null
and currentCategory
below.
components/coloring-listgroup.vue
Outdated
@@ -55,23 +61,30 @@ | |||
// 1. Get the list group item element |
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 method handles both the styling (opacity) and the category selection. I think we should split the functionality to keep things clean.
So for example: -> clicking a row only updates currentCategory
. If clickedCategory === currentCategory
, then currentCategory = null
, i.e. gets "unassigned". Otherwise currentCategory = clickedCategory
.
Then we can have a separate method called stylingCategorySelector
or something like that. This thing is responsible for redrawing the category-selector element whenever the currentCategory
changes and to apply the appropriate styling. For example, the input to the category-selector element could be a computed property then.
I think both methods/computed properties should be pretty compact and straightforward.
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 possible with the changeover to a b-table
since usage of tbody-tr-class
and @row-selected
in conjunction is possible. The only stipulation here is that currentCategory
should never be null
. This is a UX consideration.
When a user is confronted with the categorization page, one category (the first, by default) should be selected as a visual prompt for the action of selecting categories from the listbox (or table).
On top of that, the state of not being able to paint doesn't currently make sense in our current design of the page.
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 only stipulation here is that currentCategory should never be null
Now that I think about it, we currently treat null
as an (empty) category. So there could be a use case for a no-current-category-selected state where currentCategory
is null
(e.g. you navigate back to categorization page to unassign a category but now first either need to select the current category to "unpaint" it or paint and "unpaint" with a different category). But I agree, that might make the interface less intuitve. Besides, we have other ways of quickly unassigning a category (e.g. in the annotation page).
components/coloring-listgroup.vue
Outdated
let makingItemOpaque = ( this.defaultOpacity == currentOpacity || | ||
"" == currentOpacity ); | ||
let makingItemOpaque = ( this.opacities.default == currentOpacity || | ||
"" == currentOpacity ); | ||
|
||
// NOTE: Blank style string means it is unstyled. | ||
// This occurs because Vue CSS is considered to be an external stylesheet | ||
// If needs for more dynamic CSS styling arises, may need to re-address | ||
|
||
// 3. Make all list group items transparent | ||
let listGroup = document.getElementById(this.tag + "-listgroup"); |
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 don't think we need to go the DOM route here. We can use a b-table instead of the listgroup and apply our custom category CSS classes via :tbody-tr-class
. This will also be a lot more compact.
components/filedata-table.vue
Outdated
|
||
// A. Set each row to either default or custom style, depending on the category column map | ||
let rowKey = this.tableID + "__row_" + index.toString(); | ||
let row = document.getElementById(rowKey); |
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.
Here too I think we do not have to go the DOM route. :tbody-tr-class
is a more concise way to apply classes to the rows (and I think we had that before, no?). We can set the selected-variant = ""
to an empty string so we don't have to worry about the default selected styling.
components/filedata-table.vue
Outdated
methods: { | ||
methods: { | ||
|
||
styleTable() { |
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.
nice! I like this method!
But why isn't it used/called inside this component? Looks like this is being invoked only from the categorization page via reference. If this is supposed to restyle based on state changes, we can just pass the global state variables via props and use the :tbody-tr-class
in a computed property that'll update automatically.
store/index.js
Outdated
|
||
// Participants.tsv file data | ||
// For format see 'convertTsvLinesToDict' in index.js | ||
tsvDataOriginal: null, |
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 rawDataTable
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 was thinking that these tsvData
fields in the store should be grouped under one object. How about something like:
tsvData: {
raw: null,
annotated: []
}
And a similar object for the dataDictionary
fields. I'll just mention this here instead of replying to each of your comments on the store names.
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.
oh, yeah, I think that's a better way of handling this, agreed! In this case I would still drop the file extension from the variable name. Maybe dataTable
and dataDictionary
?
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
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 an addendum: How about dataTable.original
instead of dataTable.raw
? Raw data is not actually stored here as it's converted into the format (array of objects) asked for by b-table
?
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.
agreed, that's clearere!
store/index.js
Outdated
// Stores table data in format ready for Bootstrap table | ||
// This is an array of objects. See 'tableDataFromTsvAndOrJson' in | ||
// categorization.vue for exact format | ||
tsvDataTableFormatted: [], |
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 annotatedDataTable
store/index.js
Outdated
|
||
// Original data dictionary file data | ||
// (formerly home.jsonFile) | ||
dataDictionaryOriginal: null, |
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 rawDataDictionary
store/index.js
Outdated
dataDictionaryOriginal: null, | ||
|
||
// User-amended data dictionary file data | ||
dataDictionaryAmended: {}, |
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 annotatedDataDictionary
store/index.js
Outdated
// Maps our categories in 'categoryList' to colors in 'toolColorPalette' | ||
// (Final class names pending). This way colors can be swappable and | ||
// rearrangeable for categories | ||
categoryToColorMap: { |
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 believe we wanted to turn this into a computed property (via a hook on the upload page) and have an explicit mapping of category names to color names. This will allow us to access this more intuitively inside the app (e.g. by searching for the category name as the key rather than an index we have to get from some other place first.
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 in other words, depending on the flavor of annotation the category to color mapping changes? I think we discussed this but was not considered for this PR. Since I also recall us discussing putting that on a page that will precede the upload page, I think we should make an issue for this feature. I've created one here: #67
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.
OK, makes sense to handle this in a separate PR. I mainly brought it up here because I find it more intuitive if categoryToColorMap
has the actual category names (like categoryColumnMap
does), i.e.:
categoryToColorMap: {
"Age": "color1",
"Sex": ...
}
I remember we discussed the computed property both in the context of future flexibility (for other categories), but also as a way to get the above explicit mapping without having to retype the category names a second time (and having to keep them in sync). I was more referring to the clarity aspect here. I'll add it to the issue.
@surchs While a Though the reasoning for the use of class swapping via DOM selection instead of When we use See my codepen here, adapted from the one you posted here yesterday: https://codepen.io/jarmoza/pen/RwjXYrq So either we can live with the solution of selecting the DOM element and swapping the class or find another workaround in Bootstrap-Vue. I'll refrain from commenting further on your comments that address that |
For reference, The So, short story: |
In addition, via this morning's update by @surchs. Though |
It appears that It's not entirely clear why this codepen works but e.g. this very similar one does not. Rather than digging deeper into the details of the Vue implementations in these web services or Vue itself, we tried out the working solution inside our Vue app and found that it does work. So we decided to go with it. |
…k of global store
@surchs Here are notes for updates made in commit 4be1cca Components category-select-table: This is the former column-linking-table: This is the former
tool-navbar:
Pages annotation: The annotation placeholder page has just been updated to use the changed categorization:
index:
Vuex Store index.js: Store has been reworked and extraneous fields and functions been removed. In particular,
Main.css Some clean up and introduction of |
…g with category data initalization
Changes per our discussion yesterday plus some changes that made sense to do now: the introduction of the chance to call initialization code for the store and dynamic set up of category data structures.
|
Re: where the input categories come from/are initially stored. It's pretty much up in the air. If you have any opinions or preferences, let me know. |
@jarmoza: about to review the new changes, I get a crash when I navigate upload > categorization:
Could you check if the branch runs for you or you may be missing some commits? |
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 good stuff. I really like the way the store is looking now, it all feels a lot cleaner and more accessible. Now that we have this great global store, I think we can save some space with the local store implementation in the index.vue and categorization.vue components. I have left some comments but I will make my suggestions via commits because I think that'll be easier.
I also noted that the setupColumnToCategoryTable
method on the categorization component has some repeated sections left over but I will make my suggestions in a second commit - otherwise this might get a bit lengthy.
Looking forward to having this all merged soom! 🎉
} | ||
}, | ||
|
||
props: ["categories", "categoryClasses", "instructions", "title"] |
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.
Let's give these props a type and also define the ones that are required. See also the Vue style guide: https://v2.vuejs.org/v2/style-guide/?redirect=true#Prop-definitions-essential
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 good stuff.
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: #70
components/category-select-table.vue
Outdated
selectCategory(p_row) { | ||
|
||
// If a new category was selected... | ||
if ( p_row.length ) { |
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 understand that 0
is falsey, are you OK with being more explicit and typing out the !== 0
part here?
} | ||
}, | ||
|
||
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.
here too I'd say let's type these and declare the required props.
components/column-linking-table.vue
Outdated
this.$forceUpdate(); | ||
|
||
// B. Tell parent page that drawing request has been made | ||
this.$emit("done-redraw"); |
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 don't understand why we need this watcher. I think with our current solution for the column-linking-table, everything should be reactive. And this should include the props being passed to the component. So even if something is changing in the global store, this component should automatically refresh.
If this watcher (and the references to it elsewhere) are just left over from when the reactivity had to be forced, let's remove it. If you think this construct is still needed, could you please explain what would not work if we removed it?
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 double-checked. This can be removed! 🥳
}, | ||
|
||
computed: { | ||
|
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.
// mapState will make the global store components defined in the array available inside this component under the same name | |
// see https://vuex.vuejs.org/guide/state.html#the-mapstate-helper for details |
Let's add a comment here to explain what these strings mean and where we would look them up
color4: "category-style-3", | ||
color5: "category-style-4", | ||
colorDefault: "category-style-default" | ||
} |
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.
very nice and clean stuff. I really like the way the store looks now!
|
||
// 2. Save either an empty array or array of tsv dictionaries to state data | ||
p_context.commit("setTsvFile", newTsvData); | ||
initializeCategories(p_context, p_categories) { |
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 linter shows this as unused. Maybe it is called from outside vue?
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 actually a function for future action calls. initializeCategories
will be called when we want to change category lists for different annotators. Since I was implementing dynamic category setup and creating a mutation (setupCategories
) for this purpose, a corresponding action accessible by the pages (the one that will be swapping category lists) makes sense.
p_context.commit("setupCategories", p_categories); | ||
}, | ||
|
||
nuxtServerInit({ 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.
linter also says this is unused. I assume that's because it called by the Nuxt server?
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, that's correct. We will not explicitly call this, but rather Nuxt will.
|
||
return ( p_state.painting.palette.backgroundColors.length > 0 ); | ||
}, | ||
isColumnLinkedToCategory: (p_state) => (p_matchData) => { |
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 also shows up as unused for me.
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 was wavering about whether or not to take this one out. One can check whether a column is linked to a category via the store by referencing columnToCategoryMap
via mapState
, but by keeping this function we can also use it to ask this question of the store and not have our pages/components worry about what that check entails.
This is generally considered good programming practice where the details of conditional checks are kept close to their data sources. In fact, I was thinking we may want to consider doing this throughout the app where applicable. And there are other instances of this scenario pops up. (See isDataTableLoaded
and isDataDictionaryLoaded
, for instance.)
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 have made an issue to discuss this potential refactor: #72
@@ -336,7 +361,7 @@ function convertTsvLinesToDict(p_tsvLines){ | |||
// 2. Create dictionaries for each tsv row keyed on the column headers | |||
for ( let index = 1; index < p_tsvLines.length; index++ ){ | |||
|
|||
let tsvRowDict = {} |
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.
commenting here because I cannot comment on unchanged code: the function printArray
above seems to be unused according to my linter.
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, it was a debug function I had been using. It can be removed if you prefer.
Ah, sorry about that. My silly code reformatter has ruined the diff. Hope you can still make out the suggestions. |
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.
OK @jarmoza, as we just discussed, let's merge this now and turn the remaining comments into their own issues for later.
As per #64, this PR contains the proposed, new version of the global store and new means of dynamic styling (via store data and class names only) for category linking on the categorization page.
Some detailed notes
store.pageData
. It made sense to do this conversion in this PR because currently the majority of the store serves category-column linking and stylingpaintingData
). This appears to be the best way to do this since b-table somewhat obscures access to the table rows and its dynamic class attribute for its rows (tbody-tr-class
) is only computed on page load.$refs
was actually only used once by the categorization page itself (seestyleTable
, essentially a pass-through function). This is to ensure proper code encapsulation when the page changes state – e.g. the page knows its child component (the table) needs to be refreshed and directs it to do so. In this case, it's thefiledata-table
needing to repaint itself when the categorization page is navigated back to from another pagecoloring-listgroup
and technically, rendering the current category transparent, and thus appearing as if painting the table was disabled. Now subsequent clicks on the already-selected category in the listgroup produces no visible change.