Page state preservation for Categorization page, Palette/Table clicking simplification, and general cleanup for landing and categorization pages #61
Conversation
…lette on page load and placement in store, general cleanup and streamlining of landing and categorization pages/components
There was a problem hiding this comment.
Thanks @jarmoza for the PR. I took some time to go through the changes and also tried to understand the logic for state and coloring.
My takeaway is that we really need to rethink our handling of the coloring and the categorization of column names. As in: we need to split the two problems and handle them separately. At the moment, category assignment and also the "is categorized" status of columns is tracked via their style color attributes in the DOM, and this is also reflected in the function and variable naming throughout the app ("paintTable", "paintingData"). From what I can see, routing everything through the color requires a very considerable amount of code complexity (regex parsing of our CSS stylesheet, specific lifecycle hooks, comparing colors to infer categorization status), some of it undesirable (direct editing of DOM elements to set color rather than letting Vue handle it).
Our main goal is to get users to tell us what categories each column and value belong to. The colors are just a visual guide / helper and should not force us to adopt complex design. I think if we dropped colors alltogether, we could probably build a simple mapping of column names to categories just via the row-click events of our tables in a few lines of intuitive code.
From there we can think about applying color. If I understand things right, coloring the row tables is a bit tricky because we are using bootstrap and they are opinionated about colors and themes. I saw that in the current master (before this PR), you had used :tbody-tr-class to programmatically style the table rows. If that works, I think we should figure out the categorization, have a mapping of categories to css classes (hardcoded), and then feed the appropriate array to :tbody-tr-class every time a categorization update has happened. If :tbody-tr-class is not a viable solution, then we should look into b-table-simple instead of b-table. It is a bit more complex and less adaptable, but we gain fine-grained control over all elements of the table and if that allows us to lose complexity elsewhere, I think it's still worth it.
So my suggestion is:
- let's split categorization from coloring,
- ignore the coloring for now,
- figure out fundamentally the categorization and state keeping for the categorization,
- refactor variables and function names to unambiguously identify them as about categorization
- remove code about coloring aggressively and focus on reducing complexity
- once the categorization is working (with or without coloring), think about how we can style the tables with color
This may make sense to do in a separate PR, in which case I'd say we close this one and have a chat. Let me know what you think.
| let row = document.getElementById(rowKey); | ||
|
|
||
| // II. Repaint the row the corresponding colors | ||
| row.style.backgroundColor = paintingData[columnID].bColor; |
There was a problem hiding this comment.
I don't think this is an improvement to the previous painting system via :tbody-tr-class. Now we are editing the DOM directly instead of letting vue handle that part. If there is no easy way to color a table row through vue bootstrap using b-table programmatically, then we should either stick with the row-styling via css classes or move to b-table-simple to have more control over the row styling.
| @@ -9,8 +9,7 @@ | |||
| :items="tableData" | |||
| primary-key="primary-key" | |||
| @row-clicked="tableRowClick" | |||
| select-mode="multi" | |||
| :tbody-tr-class="paintClass"> | |||
| select-mode="multi"> | |||
There was a problem hiding this comment.
As noted below, I don't think getting rid of :tbody-tr-class in favour of direct DOM editing is the right choice here.
| // 0. Counts number of painted columns | ||
| let columnCount = 0; | ||
|
|
||
| for ( let columnName in paintingData ) { | ||
|
|
||
| if ( defaultBackgroundColor != paintingData[columnName].bColor || | ||
| defaultForegroundColor != paintingData[columnName].fColor ) { | ||
| if ( this.defaultBackgroundColor != paintingData[columnName].bColor || |
There was a problem hiding this comment.
I really think we need to separate the categorization (and how we store categorization information in the store) from anything related to color, styling, and CSS. At the moment, if I understand things correctly, we have a global store called pageData.categoriztion.paintingData that keeps track of tsvCategory and fColor and bColor and primaryKey for the rows that have been annotated.
And here we are counting columns that have been annotated with countPaintedColumns by comparing their color names to a default color name. But the color is just a visual guide, the important information is what category a column has been mapped to. So I really think that routing everything through the color adds a lot of hard to understand complexity here that we should think about clearing up before we merge this!
| foregroundColors: foregroundColors | ||
| }); | ||
| }, | ||
|
|
There was a problem hiding this comment.
If I understand this correctly, the entire above added section (retrievePalette) exists in order to regex parse the main.css stylesheet in order to retrieve the colors we have defined there and store them in a global store backgroundColors and foregroundColors. And if I also understand this correctly, the only reason we need to do that is because we keep track of what category a column has been assigned to by comparing it's current DOM style color attribute to the values in this list.
As I mentioned above, I really think we need to completely split colors and categorization and focus on keeping track of the categorization.
mounted()in each page) from stylesheet and placement of palette in store for later use by JS componentssetCurrentPaintClass(0)incategorization.vue) and placed in the storepaintTable()incategorization.vue)