Use new FacetedPlot variants

[chowington]

Fixes VEuPathDB/web-components#259

A couple of outstanding tasks:

• Center the modal plots better
• Fix the broken modal close button icon

[chowington]

@bobular @dmfalke Do y'all see the close button icon at the top right of the modal or is it missing for you as well? Here's what it looks like for me:

[chowington]

I also noticed that the mosaic y-axis labels are overflowing their container again sometimes, after multiple attempts on my part to fix that. It'll have to be a separate ticket since I've already merged the web-components portion of this work.

Edit: I had to do some more web-components work, so I pushed a fix for this too 👍

[moontrip]

@chowington Yes the close (X) icon is not shown for me either, however, this works for me (use fa instead of fas)
<i class="fa fa-times fa-3x"></i>

Intentionally used fa-3x for checking it: you can use fa-lg, fa-2x, etc for sure :) - tested at both chrome and firefox

[chowington]

That's simple enough! Thanks, DK!

[moontrip]

@chowington as for vertical centering, though there may be a better approach, one way is to add flex and align-items for the following div class,

<div class="ReactModal__Content ReactModal__Content--after-open" ...>

so, like adding the following two css properties in there.

display: flex
align-items: center

This will make vertical center as screenshot below (worked at both chrome and firefox), but be caution that this may affect all react-modal without specifically pointing out the target as you know well :)

I didn't check it carefully but this means that perhaps it can be configured/controlled from Mike's FullScreenModal component ? I mean, since it utilizes react-modal, there may be a way to set vertical center: looks like customStyles at the module's example may also align horizontally and vertically ?

[moontrip]

@chowington Sorry I forgot to say one additional change at FacetedPlot component in conjunction with above. Wrapping modalPlot with div as well. So like,

      {modalComponentProps && (
        <FullScreenModal visible={modalIsOpen}>
          <button
            onClick={() => setModalIsOpen(false)}
            style={{
              position: 'absolute',
              top: 30,
              right: 30,
              backgroundColor: 'white',
              cursor: 'pointer',
              border: 'none',
              zIndex: 2000,
            }}
            title="Close expanded plot"
          >
            <i className="fas fa-times fa-lg"></i>
          </button>
          <div> {/* DK: here */}
            {modalPlot}
          </div> {/* DK: here */}
        </FullScreenModal>
      )}

[chowington]

Thanks DK! That worked really well

[chowington]

PR is ready for review! @dmfalke @bobular @moontrip

[chowington]

In this commit (6af36b3) I fixed some unrelated TS warnings in the same files, and in the process I noticed that adding some dependencies to dependency arrays as TS recommends actually breaks the viz. A couple of comments already in the code hinted at that. I only committed the changes that didn't seem to break the code, but if anyone else is more familiar please do review the changes I made.

[moontrip]

@chowington I have just tested two cases, vertical center and X icon and they worked fine: I like the full screen modal to see a big plot 😄

Certainly not for this ticket but it comes to my mind that such a full screen modal would be quite useful for non-faceted plots as well, especially the cases that lots of stratifications make one difficult to see details in a normal small-size plot. In my opinion, it deserves to be a new task :)

[chowington]

@chowington I have just tested two cases, vertical center and X icon and they worked fine: I like the full screen modal to see a big plot 😄

Certainly not for this ticket but it comes to my mind that such a full screen modal would be quite useful for non-faceted plots as well, especially the cases that lots of stratifications make one difficult to see details in a normal small-size plot. In my opinion, it deserves to be a new task :)

Thanks for the approval. I could see that being useful as well, DK!

[bobular]

Apart from the interactive thing discussed on slack, this looks great.

Do you intend the mosaic legend/axis labels to be all bunched up? I think they look better spaced out.

Also, I'm curious what DK's div wrapping suggestion did? @moontrip

[chowington]

Apart from the interactive thing discussed on slack, this looks great.

Do you intend the mosaic legend/axis labels to be all bunched up? I think they look better spaced out.

Also, I'm curious what DK's div wrapping suggestion did? @moontrip

@bobular They're bunched up because containerStyles.height = '100%'. I couldn't find a way to determine the plot height in px from non-px values, which is required to know how tall the not-legend can be, so the safe fallback is to not space them out at all

[moontrip]

Hi @bobular 👋 I saw you called me 😄

So, when using display:flex, it consists of flex container (so-called parent) and child div(s). It is often obscure which ones should be flex container (parent) and children, especially when using library-based component in React. So, simply put, the

that wraps plotly plot is a sort of declaration/clarification that it is a child of a flex container.

[chowington]

@bobular How about my lastest commit? Does that work for you?

[chowington]

The faceted plot spacing isn't perfect, but this needs to go ahead and get merged. We can add another ticket for the plot spacing if we need to. Merging now.