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
7 stars 9 forks source link

Whitespace on component could be reduced #59

Closed pacharanero closed 5 months ago

pacharanero commented 11 months ago

Looking over the React component today I noticed how much white space there is, and how this has the effect of reducing the overall size of the chart as compared to the container it is in.

I wondered if it would be possible to move some of the logos and buttons to give an overall 'tighter' feel, reduce whitespace, and thereby scale up the chart itself for any given window. I think this would make the clinical view better and might also give us the opportunity to create a 'toolbar' along the bottom for all the buttons we have.

Current whitespace

image

Proposed moves of logos/buttons/text

image

@eatyourpeas any thoughts on this?

eatyourpeas commented 11 months ago

The whitespace below the charts fills with buttons if there is age correction and to the right, with the reset zoom button if there are data points. The chart itself is an SVG with all the other components (the images and buttons actually sitting around the edge. If we sit them on the chart, it would interfere with the interactivity - eg if the chart is zoomed in, the images and buttons would potentially be in the way. So generally I would advise against having things ontop of the chart itself. The container the chart is in could be resize though so there was less space off to the sides. I have also actually put a new label for the reference under the x axis label in the latest version, so we could potentially deprecate the title. Currently the title and subtitle are props so the users can pass in the name of the child for example to include. We could instead have a title we prescribe, lose the subtitle and have the users pass in the patient name/identifier as an optional Perhaps one to talk about?

pacharanero commented 11 months ago

The whitespace below the charts fills with buttons if there is age correction and to the right, with the reset zoom button if there are data points.

OK so we could perhaps consolidate the style and location of these buttons into a more regular 'toolbar', even if it still adapts to what is displayed by adding and removing buttons dynamically? It just seems like we could make it more regular instead of having 2 or 3 types of button style.

So generally I would advise against having things ontop of the chart itself.

OK, understood.

we could potentially deprecate the title.

I think still good to have title at the top in bold so it is super clear what reference and sex. But that could all be in one line easily.

We could instead have a title we prescribe, lose the subtitle and have the users pass in the patient name/identifier as an optional

Yes, ideally if that can all go on a single line then it sounds like the way to go.

raspberrycodes commented 8 months ago

Keeping an eye on this issue for our own bug logs. Thank you for looking into this.

If this helps at all: in our implementation, we are presenting the react component in a pane that is 2/3rds of the width of the page. Our input form we have on the other third of the page goes down the page a little bit, so we have a scroll bar on the page. Therefore, we are more limited by the whitespace on the sides of the chart, than any space taken up above and below the chart. As long as the chart itself does not need the user the scroll up and down the page to see it, we would really appreciate being able to make it wider (we will do this our end potentially).

pacharanero commented 7 months ago

Thanks @raspberrycodes - I was just revisiting ths Issue to add the NHS Wales request which is that it would be nice if the whitespace could be reduced. We will be working on this over the next few weeks.

roraspberry commented 7 months ago

Thank you Marcus!

pacharanero commented 6 months ago

Further to recent meeting with NHS Wales last week - they have reiterated their request for reduction in the whitespace in the react component.

eatyourpeas commented 5 months ago

Fixed and merged in v7.0.0. Documentation updated