search

VEuPathDB / web-components

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

Monorepo pre-migration #425

Closed jernestmyers closed 1 year ago

bobular commented 1 year ago

Hi Jeremy, I'm not sure why, but I was having a look at this. Before I realised it was still in draft.

I guess was interested because I considered using react-html-parser in one of my PRs: https://github.com/VEuPathDB/web-components/pull/422 - and decided to dangerously set the inner HTML instead. The HTML in question there is all client-generated SVG with no user- or database-derived content, so no security issues as far as I know.

Anyway... I looked at this PR. Was the previous HTML parsing used to render the <br>s joining the labels, or potential italic tags in the labels?

Also, I see that there was, and is, code for summing up counts for items beyond a hard-coded limit of MAXIMUM_LEGEND_LIST_ITEMS = 10 and displaying it as an "Others" category.

As far as I know, all such behaviour is handled by the back end (for the map categorical overlay) and other overlays (e.g. legends) are limited to 8 values by the back end-provided constraints and front-end enforcement of these.

So I think this code could be an vestigial relic of early versions of the legend that we were making in storybookland before we had the EDA back end. If so, it would be good to remove it to avoid it being maintained in the future.

@moontrip would you mind commenting please?

jernestmyers commented 1 year ago

Jamie recommended finding a replacement to react-html-parser as part of the monorepo's pre-migration tasks, though I'm not entirely sure why. When I looked up its use, I came across only this particular component and it seems unnecessary. The component is simply constructing a string of HTML, but we can just leverage JSX instead.

It's unclear to me why we opted for react-html-parser, so I'd be curious what @moontrip knows about this component. It looked like he was offline Friday when I was working on it, so thanks for catalyzing this conversation.

Regarding the limit of legend items, I'll have to gain more clarity on that. The data coming into LegendListGeneral often had many more than 8 values and lacked any sort of summation.

moontrip commented 1 year ago

Hi @bobular and @jernestmyers Yes the LegendListGeneral and some of similar components were very initial works to test and demo react-leaflet with a legend, which mimicked old VectorBase map without thinking of any backends. Thus they have not been used except some early stories, so almost obsolete. As you know, now we are using a custom legend even at FSM :)

If I remember correctly, yes I tried react-html-parser instead of dangerouslySetInnerHTML because it offered more flexibility, though that feature was not used in this legend haha.

dmfalke commented 1 year ago

The HTML in question there is all client-generated SVG with no user- or database-derived content, so no security issues as far as I know

@bobular you ought to be able to use JSX for svg. I don't know of any reason to construct an HTML string in client client and pass to dangerouslySetInnerHtml, unless we are adding HTML content from an external source. I'm happy to be proven wrong, though!

bobular commented 1 year ago

The HTML in question there is all client-generated SVG with no user- or database-derived content, so no security issues as far as I know

@bobular you ought to be able to use JSX for svg. I don't know of any reason to construct an HTML string in client client and pass to dangerouslySetInnerHtml, unless we are adding HTML content from an external source. I'm happy to be proven wrong, though!

Ah, that's a great idea. This some of our earliest code. I think it should be a separate tech debt ticket though, what do you think @moontrip ?

moontrip commented 1 year ago

The HTML in question there is all client-generated SVG with no user- or database-derived content, so no security issues as far as I know

@bobular you ought to be able to use JSX for svg. I don't know of any reason to construct an HTML string in client client and pass to dangerouslySetInnerHtml, unless we are adding HTML content from an external source. I'm happy to be proven wrong, though!

Ah, that's a great idea. This some of our earliest code. I think it should be a separate tech debt ticket though, what do you think @moontrip ?

Yes it was originated from Vectorbase's Popbio map, so I agree that it can be a tech debt.

bobular commented 1 year ago

Here's the new tech debt ticket https://github.com/VEuPathDB/web-components/issues/432

bobular commented 1 year ago

Hi @jernestmyers Why does this PR use the monorepo-pre-migration branch but have a seemingly unrelated PR title? I'm confused!

jernestmyers commented 1 year ago

Hi @jernestmyers Why does this PR use the monorepo-pre-migration branch but have a seemingly unrelated PR title? I'm confused!

Ah, forgot about this PR in general. The title is derived from what the initial PR actually did, as this was the first thing I addressed in web-components. But since then, it's certainly evolved into much more. I'll change the PR title.