Skip to content
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

Transparent category colors and some related UI polish for the categorization page #47

Merged
merged 7 commits into from
Feb 22, 2022

Conversation

jarmoza
Copy link
Contributor

@jarmoza jarmoza commented Feb 14, 2022

  1. The categories on the categorization page's category listbox are now always colored to indicate to the user which category corresponds with which color.
  2. The currently selected category (for 'painting' the participants.tsv/data dictionary json table opposite the category listbox) is now fully opaque while the other categories are made to be half-transparent.
  3. When created, the categorization page starts out as painting the first possible category (currently 'Subject ID').
  4. Some change to the header and descriptive instruction text to reflect our terminology change to 'category'.

@jarmoza jarmoza requested a review from surchs February 14, 2022 23:28
Base automatically changed from navigation-and-state to master February 16, 2022 21:50
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jarmoza. Looks good. I did have one comment on the colors in the style section of the coloring-listgroup.vue component. Would be nice if we can have a single source for these. Otherwise good to go!

@@ -61,8 +71,11 @@
for ( let index = 0; index < listGroup.children.length; index++ ) {

// A. Decolor the list group item
listGroup.children[index].style.backgroundColor = this.defaultPalette.bColor;
listGroup.children[index].style.color = this.defaultPalette.fColor;
// listGroup.children[index].style.backgroundColor = this.defaultPalette.bColor;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we are doing this via transparency now? Can we get rid of the default colors? And if so, update the enumeration in the comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're correct. And I had started working on that. But since the change would require some considerable working over and I wasn't sure if this would be our final change for category painting, I didn't want to go back and tackle that.

If you'd prefer, I can. But as it is right now since that information is stored as what determines the painted state of a row, it works as is regardless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I just meant the comments here. Agree, no good point in reworking the default colors here. Sorry for not being clear. So: since we now set the item to opaque and the old code is commented out, let's remove the old (commented out) code and update the II. enumeration in the comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Though since I'm reworking the color, I'm thinking I may as well rework the cue for 'coloring' (really making opaque) the list group item. Hopefully it will just take a little bit of time today.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@surchs Upon further inspection, the color cue makes sense. It is for the table that's being painted and not the list group with categories. I had mistakenly thought the stored paintingData was referencing the list group. So all good!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jarmoza, did you mean to make new commits here? I cannot see any changes to the code since the last review.
e.g. II. -> I.

components/coloring-listgroup.vue Outdated Show resolved Hide resolved
@@ -105,6 +124,37 @@

<style>

.column-paint-0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these guys are the same as recommendedCategories.backgroundColors in the categorization page. Can we find a way to only define the colors once and keep them in sync otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll be working on converting this today so pages and components read from a palette stored in a global css file for the app (stored in assets/css. This will only be helpful moving forward, so it's a good excuse to make this changeover.

Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jarmoza, this looks good from my end.
I'd say we tackle #50 in a separate PR and close this one

@jarmoza
Copy link
Contributor Author

jarmoza commented Feb 22, 2022

A new implementation for retrieving the tool's color palette has been added to the categorization page. If we need this elsewhere, we can expand it to the store for other pages.

In essence, the colors are still being stored in the categorization page's data object, but are no longer hardcoded and are instead retrieved on page load (in the "mounted" hook) from the classes being used for the category listgroup items. This logically makes sense since these are the colors that the user will be 'painting' the table with on the right side of the page.

The older implementation from a recent commit in this PR solved this by manually parsing through the styleSheets list available in the global document object. Searching for the category-style- classes being used by the listgroup items, this code then parsed the string that contained the corresponding CSS stylings for each class.

This was necessary because the style had not yet been applied to the listgroup items on the page in a place that was accessible via the Vue lifecycle (even through a forced, immediate update of the page). Instead, it appears that CSS defined in an 'external' stylesheet (in the template or actually external) is applied by the browser at a later time. Thus, document.styleSheets was the only accessible means of getting the desired CSS values (the background and text colors).

However, there is another way that's much cleaner and built into JavaScript via the global window object. We can retrieve the "computed style" – presumably what the browser will later apply to the corresponding html elements. With window.getComputedStyle we can read the colors from the listgroup items, matching them together with the category-style- classes and then fill out the background and foreground color lists stored in JavaScript for later use in the Vue code.

One other thing to note (and it's mentioned in a comment in the "mounted" hook), both the window and document objects are not yet accessible in the "created" hook, and thus this palette retrieval and initial painting color assignment happens in "mounted."

@jarmoza jarmoza merged commit 215b9b8 into master Feb 22, 2022
@surchs surchs deleted the transparent-category-colors branch February 22, 2022 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants