Skip to content

[BHP1-1317] Add Color Shading to Result Matrices#213

Merged
Kcheung42 merged 8 commits intomainfrom
kc-BHP1-1317-add-color-shading-result-matrices
May 4, 2026
Merged

[BHP1-1317] Add Color Shading to Result Matrices#213
Kcheung42 merged 8 commits intomainfrom
kc-BHP1-1317-add-color-shading-result-matrices

Conversation

@Kcheung42
Copy link
Copy Markdown
Collaborator

@Kcheung42 Kcheung42 commented Apr 20, 2026

Purpose

Add feature to color result cells by color set in the VMS for list options.

Related Issues

Closes BHP1-1317

Submission Checklist

  • Included Jira issue in the PR title (e.g. BHP1-### <title>)
  • Code passes linter rules (clj-kondo --lint components/**/src bases/**/src projects/**/src)
  • Feature(s) work when compiled (clojure -M:compile-cljs)

Testing

  1. Run this migrations script migrations.2026-04-09-add-fire-type-color-tag-set

  2. Open this worksheet:
    BHP1-1317.zip

  3. Navigate to results page and notice there is a toggle for coloring results.

  4. Toggle on and off, ensure color matches across result matrices.

  5. Change one multi valued input to a single value, should have only 1 multi valued input now

  6. Run and navigate to results page, ensure color toggle also works for 1 multi valued input. (rows should be colored)

Screenshots

image

@Kcheung42 Kcheung42 changed the title [BHP1-1317] [BHP1-1317] Add Color Shading to Result Matrices Apr 20, 2026
@rjsheperd
Copy link
Copy Markdown
Contributor

Really impressive. Nice work!

Just a few things:

projects/behave/src/cljs/behave/components/results/matrices.cljs

L60 - Looks like this might have been an attempt at a multi-function? Can you remove the extra parens?

L28, 36 - I'm a bit confused why we need to reduce over the same collection twice. Mind looking into this?

L28, 36, 152, 321 - I'm noticing an interesting pattern here of using reduce-kv in very similar ways. I like this approach of update-map used in this ClojureDocs example: https://clojuredocs.org/clojure.core/reduce-kv#example-543cd003e4b02688d208b1b4

Do you think it's worth adding this to map-utils component to more accurately describe the process here and make it a bit more reusable?

projects/behave/src/cljs/behave/wizard/subs.cljs

L486 - Filtering after the pull seems quite inefficient. Can you invert this to find all matching group variables first and then perform the filter/transform?

L507, 953 - Replace the second transformer fn with identity

@Kcheung42
Copy link
Copy Markdown
Collaborator Author

L28, 36 - I'm a bit confused why we need to reduce over the same collection twice. Mind looking into this?

The first pass was needed to look over the entire data set to set the color for a row, since the data is individual cells. the second pass is to filter out data that don't have color as well as formatting the row part of the lookup key (i.e. [row col-uuid]) to match how it will be looked up (i.e. we have multi valued input for fuel model converting raw value ["1" "2" "3" -> "FM1" "FM2" "FM3" ]). With only one pass, it may wrongfully decide that ["1" "col-gv-uuid1"] shouldn't have a color but, later on ["1" "col-gv-uuid2"] does have a color. This would mean ["1" "col-gv-uuid1"] would be left wrongfully uncolored since the entire row should be. I had to do this on a cell by cell entry instead of an entire row because the matrix-table component expects individual cells to be designated for color. That said I can refactor to by collapsing this into a single logical traversal by grouping entries by row first, then
for each row finding its color (if any) and expanding to all of that row's columns. This would avoid checking more columns when we've already found that this row should be colored.

L28, 36, 152, 321 - I'm noticing an interesting pattern here of using reduce-kv in very similar ways. I like this approach of update-map used in this ClojureDocs example: https://clojuredocs.org/clojure.core/reduce-kv#example-543cd003e4b02688d208b1b4

  • L28, 36, 152 these don't quite follow the pattern for this update-map fn since I'm not expecting to have the same amount of data when it passes through this reduce. These lines are pruning entries that don't have color entry in the lookupmap. This would make sence for 321 though.

so here's proposal for ln 321, see map-utils.core/update-map

L486 - Filtering after the pull seems quite inefficient. Can you invert this to find all matching group variables first and then perform the filter/transform?

  • simplified pull so it walks on the only needed leaf attrs
  • use keep to fuse the filter and the map operation

L507, 953 - Replace the second transformer fn with identity
not sure i can use identity for 953, it needs to return a boolean and not the actual count (pos? ccount)

@rjsheperd
Copy link
Copy Markdown
Contributor

rjsheperd commented Apr 30, 2026

Nice. Thanks for adding the update-map function, making the coloring logic a bit more performant, and simplifying the pull to avoid additional filtering.

One last thing:

projects/behave/src/cljs/behave/components/results/matrices.cljs

L328 - Remove commented function (not sure why this wasn't picked up by clj-kondo)

@rjsheperd
Copy link
Copy Markdown
Contributor

LGTM 👍

@Kcheung42 Kcheung42 merged commit d38140e into main May 4, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants