REF: Adds 4th dimension of Sample_Size to Heatmap

[cherman2]

No description provided.

[cherman2]

failing will be fixed by #78

[cherman2]

Hi @lizgehret,
This passes locally and the viz has been tested locally. I am now assigning you because this PR is ready

[lizgehret]

CI failures aren't interesting (see related PR here). i'm planning to do a thorough review this by EOW @cherman2, thanks for your patience!

[lizgehret]

Okay @cherman2, finally reviewing this (sorry for the delay)!

Code review all looks reasonable - just pulled down locally to do a user review, and I think all looks as expected. Here's what I see in this updated heatmap (when Sample_Size is enabled):

I think my only comment about this would be to add some sort of a description about what the Sample_Size toggle is doing (either in the plugin setup or potentially the viz itself, although that might be overkill)? Maybe that's something that would be obvious to FMT users though, so take that with a grain of salt!

Otherwise this all lgtm 🙂

[cherman2]

You are totally right @lizgehret I should explain sample-size somewhere. I think I am going to do it in my general description of the method because it isn't a param that is passed so I can't do it as a param description (which location wise would make the most sense to me).

looking at your screenshot I think I want to rename Sample_Size to samplesize to match rowheight and columnwidth. What do you think about that?

• add samplesize explanation
• change samplesize name

[lizgehret]

looking at your screenshot I think I want to rename Sample_Size to samplesize to match rowheight and columnwidth. What do you think about that?

I think that's reasonable! Maybe it's worth utilizing JS formatting for those variables (i.e. sampleSize, rowHeight, columnWidth) just so there's more of a distinction between words?

[cherman2]

On it! Thanks for the feedback!

[ebolyen]

Just tested locally as well. @cherman2 did you want to add the description of sample size? (you can also add this in the index.html template)

[lizgehret]

updated labels lgtm @cherman2 - just echoing Evan, do you want to add that description in this PR or do that in a separate one? i can get this merged if you want to do that separately; just lmk what your plan is!

[cherman2]

Hi @lizgehret,
Finally addressesd the samplesize explaination. Could you give this a final review when you have a second?

[lizgehret]

tested locally and the sampleSize description looks good!

one thing i noticed while testing locally is that now the x_label and y_label signals are the only params without camelCase. I don't have strong feelings about this one, but you could change those to xLabel & yLabel if you wanted everything to have the same format.

i'm happy to merge this as-is, so you just lmk what you'd prefer!

[cherman2]

@lizgehret, I did that little fix you suggested. I assume you will merge this pr in so I am re-assigning! thanks for your feedback.

[lizgehret]

LGTM! 🚀