search

rcpch / digital-growth-charts-react-component-library

A typescript React library for displaying RCPCH Digital Growth Charts from API data
https://growth.rcpch.ac.uk
MIT License
8 stars 11 forks source link

Fix-reopened-measurement-plot-size-issue-94 #97

Closed eatyourpeas closed 3 months ago

eatyourpeas commented 3 months ago

Overview

Solution for #94 FIxes the default size of the measurement points. Also refactors styles to have montserrat as a string.

Code changes

Increases both the default size and the size when crowded. Currently though that if only some measurements are crowded, the size change applies to all. Hopefully this will matter less now that they are all much bigger.

mbarton commented 3 months ago

Please add some before/after screenshots @eatyourpeas.

Also refactors styles to have montserrat as a string

Which bit does this? I could only see removing that SASS import.

eatyourpeas commented 3 months ago

I have added a point to the default size from 3.5 to 4.5. In addition, I have bumped the size for the setting for crowded points.

The sass bit was a mistake i made where i imported the font incorrectly. In @dmc-cambric image I can see that the tooltip has times new roman rather than montserrat. I am not clear why it is loading montserrat in our client, but not for @dmc-cambric. The font is part of the library resources so I am not clear why it is loading for us but not for him. I am not sure this issue is fixed.

before:

image

after:

image
mbarton commented 3 months ago

I am not clear why it is loading montserrat in our client, but not for @dmc-cambric

Content Security Policy potentially?

dmc-cambric commented 3 months ago

@eatyourpeas @mbarton seems like its set to use Montserrat (see screenshot). However, I am installing the package via. npm, is it possible that these font assets aren't included in the npm package? Might explain the difference in behaviour.

I don't see any errors in console or the network trace that our app is trying and failing to access these font assets, so don't think its a content security policy issue. Have additionally experimented with removing content-security-policy header in web app, still loads the same as attached screenshot

image

eatyourpeas commented 3 months ago

I can't explain it. The fonts are included in the build here and the rollup config. In our client which does seem to render Montserrat I don't think we include the fonts in the build there.

mbarton commented 3 months ago

In our client which does seem to render Montserrat I don't think we include the fonts in the build there.

In the vite branch we've pulled it from the CDN: https://github.com/rcpch/digital-growth-charts-react-client/blob/42177102b0decd97f4583b58782208a3ae4f4d13/src/App.css#L1.

If I remove that the chart doesn't use Montserrat so I think it's not embedding in the build correctly. The TTF files are in the bundle: https://www.npmjs.com/package/@rcpch/digital-growth-charts-react-component-library?activeTab=code. Could it be missing importing them in the chart component styles?