VEuPathDB / web-eda

Web browser code for EDA-based applications
Apache License 2.0
0 stars 0 forks source link

Mosaic 2x2 stats table #1625

Closed moontrip closed 1 year ago

moontrip commented 1 year ago

This PR intends to resolve https://github.com/VEuPathDB/web-eda/issues/1384.

As a first step, a groundwork for typing the new Mosaic 2x2 stats table (backend response) is made. Since backend response should be established for this work, this PR is branched out from Jeremy's nice PR (https://github.com/VEuPathDB/web-eda/pull/1608) (update/2x2-params-1575 branch). So, for now, the base branch is set to update/2x2-params-1575 but it may be changed to main branch later if update/2x2-params-1575 is merged.

It seems to work fine to see correct stats table props with this PR, so perhaps we just need to make a table (with requested styles) and map the props to corresponding table cells. Just in case, I am leaving a screenshot of the requested table format in the following:

mosaic 2x2 stat table itself

moontrip commented 1 year ago

Mosaic 2x2 Stats table is now implemented. Screenshots for both with or without facet variables are attached in the following:

jernestmyers commented 1 year ago

Looking into this now, but in the meantime we should update this code now that we have the stats tab.

moontrip commented 1 year ago

Looking into this now, but in the meantime we should update this code now that we have the stats tab.

Thanks @jernestmyers By the way, can you please elaborate what the update means?

jernestmyers commented 1 year ago

Yes, we currently don't render "Please select all *required parameters" in the 2x2 stats tab because of this code.

So we just need to remove the comment and the isTwoByTwo conditions like so:

showRequiredInputsPrompt = { !areRequiredInputsSelected }
moontrip commented 1 year ago

I'm going to approve, but I do have some other thoughts for possible improvement.

  1. When should we create a new component file? Our viz implementation files get rather large and complex. Would it help to break things off into their own file? In this case, couldn't TwoByTwoStats be its own component? This could be a topic at the next frontend dev meeting.
  2. I'm left wondering if there's a more DRY way to render both mosaic tables, though it may not be worth the debt since the styling could be tricky.
  3. I prefer the more uniform spacing that the mockup displays for the actual data than what's currently implementing. The center (95% CI) column's data feels a little jumbled because it is so close to the Value column's data. I think we should add some space between the Value and 95% CI columns in a way that tries to match the space between 95% CI and P-value.

Overall, very nice work!

@jernestmyers Thank you for your detailed comments. For your #1, yes I tend to agree that the stats table may become a separate component out of Mosaic Viz, but I will leave it as a technical debt :). For #2, yes that specific styling prevents from minimizing DRY. Finally, yes I noticed that too. It is because value and P-value are right-aligned while CI is center-aligned. Perhaps your suggestion may work by having some space. Will test it. 👍

moontrip commented 1 year ago

@jernestmyers I have added some space for 95% CI and I think that it looks better than before: a screenshot is attached in the following.

mosaic-2x2-stats-table1