Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

phenogrid multi-compare display is out of frame after monarch-app v1.8.0 #887

Open
dpavam opened this issue Nov 8, 2024 · 14 comments
Open

Comments

@dpavam
Copy link

dpavam commented Nov 8, 2024

The Phenogrid iframe embedded on the IMPC website does not display as expected after monarch-app v1.8.0.

This is evident when a large number of phenotypes (either subjects or objectSets) are passed to the phenogrid. A good example can be found here. To open the phenogrid navigate to Human diseases caused by Pparg mutations and expand the section for Pparg-Related Familial Partial Lipodystrophy. A screenshot shows the current issue, where the labels of the objectSets are not longer visible.
Phenogrid_bad_example

The expected behaviour is achieved using the same settings but with monarch-app v1.8.0. Where all the labels are visible and users are able to scroll up or down to see the entire phenogrid. Screenshot of the expected display:
Phenogrid_good_example

I have attempted manipulating the height without success. These are the current settings being used.

// Set the iframe to fill its container
      iframe.style.width = "100%";
      iframe.style.height = "1000px";
<iframe
            ref={iframeRef}
            name="pheno-multi"
            title="MultiCompare Phenogrid"
            src="https://monarchinitiative.org/phenogrid-multi-compare"
            style={{ width: "100%", height: "100%", border: "none" }}
          />
@kevinschaper kevinschaper added this to the 2024-11 Release milestone Nov 8, 2024
@ptgolden
Copy link
Member

I'm running git bisect from 1.8.0 onward to see where the change was introduced.

@ptgolden
Copy link
Member

The behavior changed with 2c878e8

@ptgolden
Copy link
Member

It's caused by removing this specific change:

2c878e8#diff-72efdd36954cb80c05a76e4695b9fe99e3422c23a9fcb57bd2d8d226b49f4bff

Removing that class does not set overflow: auto on the SVG container, making it the default overflow: visible, which hides any part of the phenogrid that exceeds the height of its container. @kevinschaper was there a reason you removed this?

@kevinschaper
Copy link
Member

My best guess is that I was looking at tables on the node page, but it's very possible that I made an irrelevant change that both broke phenogrid and didn't help the node page tables.

ptgolden pushed a commit that referenced this issue Nov 13, 2024
It was removed in 2c878e8 and broke the display of certain embedded
phenogrid widgets (see #887)
@ptgolden
Copy link
Member

By the way @dpavam, I notice from the code that generates this phenogrid that you knowingly URL encode labels: https://github.com/mpi2/impc-mousephenotype-web/blob/1c0014040fad666cb35c214eeaa8b4b91ff61f70/components/Gene/HumanDiseases/index.tsx#L74

Can you describe what the issue is if you do not encode them? If they are showing up wrong, that's a bug on our end.

kevinschaper pushed a commit that referenced this issue Nov 15, 2024
It was removed in 2c878e8 and broke the display of certain embedded
phenogrid widgets (see #887)

### Related issues

- Addresses #887 (though keep open until new release is deployed)

Co-authored-by: Patrick Golden <[email protected]>
@dpavam
Copy link
Author

dpavam commented Nov 15, 2024

Hi @ptgolden, this is related to #826. The mouse model identifiers (what we pass as labels to the phenogrid) contain characters such as "&", "<" or ">". If we don't encode, these characters are not shown on the tooltip.

For example: if the label is: 66.45-Dsg2<tm1a(EUCOMM)Wtsi>/Dsg2<tm1a(EUCOMM)Wtsi>

If we don't encode: the tooltip does not show the characters (and anything in between) but the label at the top is ok:
Screenshot 2024-11-15 at 18 14 59

if we encode: the tooltip shows the expected text but the label at the top is not quite right. This was a compromise we chose until the tooltip and the label have the same encoding.
Screenshot 2024-11-15 at 18 15 23

@ptgolden
Copy link
Member

Ah! Thank you. I will add that that case to our testing and fix next week.

@ptgolden
Copy link
Member

@dpavam the bug to watch is #902

@ptgolden
Copy link
Member

@dpavam after #908 is merged, you'll be able to pass text without escaping it.

@dpavam
Copy link
Author

dpavam commented Nov 21, 2024

Thank you very much @ptgolden! Out of curiosity, will it be possible to display certain labels of the phenogrid in bold? Perhaps sending something along the lines of: `<strong>${id}</strong>`?

@ptgolden
Copy link
Member

ptgolden commented Dec 3, 2024

@dpavam this issue is resolved as of the latest release-- can you confirm it's working OK? I'll close this issue if so. (You also should not have to escape any text!)

Out of curiosity, will it be possible to display certain labels of the phenogrid in bold? Perhaps sending something along the lines of: <strong>${id}</strong>?

With the changes in #908, <strong> (or, more likely, <b>) would be an easy addition. Before adding that, though, let me talk to @kevinschaper about what we want to allow and how to document it. I'll make a ticket that you can track 👍

@ptgolden
Copy link
Member

ptgolden commented Dec 3, 2024

Can you tell me the context in which you'd like to have bolded text?

@dpavam
Copy link
Author

dpavam commented Dec 4, 2024

@ptgolden thanks a lot! We applied these changes to our dev site yesterday and seem to be working fine. I will let you know when we move them to production.

The phenogrid currently presents mouse model descriptions on the column labels (objectSets), these models might come from different sources like MGI or IMPC. We want to mark those IMPC models only in bold so users can distinguish from the rest them easily.

On our side, we can make that distinction clear to the phenogrid by adding a specific character/marker to the labels of each objectSet so that it knows those specific labels have to be bold.

@dpavam
Copy link
Author

dpavam commented Dec 9, 2024

@ptgolden this issue is solved. Thank you very much!

We noticed a small bug related to #755 after clicking transpose or sort. The tooltips of the labels do not move accordingly. The tooltips on the cells are now ok. See images attached.
Screenshot 2024-12-09 at 15 42 20
Screenshot 2024-12-09 at 15 42 33

You can see the issue:

  1. Visit https://www.mousephenotype.org/data/genes/MGI:1196466
  2. Navigate to the Human diseases caused by Dsg2 mutations section
  3. Look for the disease Arrhythmogenic Right Ventricular Dysplasia, Familial, 10 and click expand
  4. Check the label tooltips, then click transpose and that should replicate the screenshots.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants