search

VEuPathDB / web-components

Library of React components for plotting data
Apache License 2.0
1 stars 0 forks source link

Mosaic long axis title #423

Closed moontrip closed 1 year ago

moontrip commented 1 year ago

This will address https://github.com/VEuPathDB/web-eda/issues/1377. Since there is another mosaic work by Connor, https://github.com/VEuPathDB/web-components/pull/418, this is branched out from mosaic-elbow-labels to minimize conflicts and accommodate new layouts. Thus, this PR will be finalized when https://github.com/VEuPathDB/web-components/pull/418 is done.

Now both X and Y axes have ellipsis. For this, the max length of X-axis string is set to 80. Also, a story is made for testing. Here is a screenshot.

mosaic-axis-ellipsis

moontrip commented 1 year ago

@chowington I was waiting for your elbow work to be done for this PR :) Since it is finished, I updated this PR and changed it to be ready for review

chowington commented 1 year ago

Hi DK, thanks for this work! While testing I found a case where the title still overflows:

image

This is on the WASH Benefits Bangladesh Cluster Randomized Trial study. Can you look into this case? I wonder whether it would be worth it to have the title length dependent on the plot container size? That gets hairy sometimes though...

chowington commented 1 year ago

Also, I'd suggest trying to DRY up this code bit. For instance, it could be helpful to factor out the ellipsis logic into a function, as well as to assign plotlyProps?.layout?.xaxis?.title to a const so you don't have to repeat it every time.

moontrip commented 1 year ago

@chowington Thank you for your reviews and tests! 👍 I have addressed them in the new commit: reduced the max length of x-axis title (attached a screenshot with the same example) and make that ellipsis of the axis title be a function, axisTitleEllipsis().

mosaic-long-axis-title1

moontrip commented 1 year ago

Looks good, thanks DK!

@chowington Thank you for your review and tests! 👍