search

palerdot / react-d3-speedometer

React Speedometer component using d3.js ⚛️
https://palerdot.in/react-d3-speedometer/
MIT License
230 stars 57 forks source link

Fluid Width prop to take only necessary height #166

Open palerdot opened 1 year ago

palerdot commented 1 year ago

PR - https://github.com/palerdot/react-d3-speedometer/pull/165

This PR makes changes to take only necessary height when fluidWidth: true is configured.

yoelbassin commented 1 year ago

I have added a new prop called valueTextBelowPos.

Previously, the value 23 was hard coded in the _renderCurrentValueText. The label was placed manually 23pt below to gauge. I propose extracting this value into a configuration, so it won't be a 'magic number'.

This value is needed in this PR, since I calculate the minimum height of the gauge along the label.

I could 'magically' add 23pt to the height of the component, but that wouldn't be correct.

If you think that it is correct, I will update the documentation and test as you mentioned in #153

palerdot commented 1 year ago

I have added a new prop called valueTextBelowPos.

Previously, the value 23 was hard coded in the _renderCurrentValueText. The label was placed manually 23pt below to gauge. I propose extracting this value into a configuration, so it won't be a 'magic number'.

As a start, the prop name should be changed, to valueTextYOffset. The current prop is not descriptive and confusing as a public facing prop/api. Since we are dealing with height/Y axis distance, we should name something along those lines like valueTextYOffset. In future, there might be a similar valueTextXOffset position.

We also need to consider what happens if the user (rightly) gives a negative value like -23. Will the text be automatically aligned at the top as expected? Please check on this and may post a screenshot of this scenario with this code changes.

There are other code changes which I will comment in the PR. For example, there is duplication of parentHeight - PROPS.valueTextBelowPos, which should be put in a variable like fluidHeight / calculatedHeight, changing {width: width} to {width} etc.

If you think that it is correct, I will update the documentation and test as you mentioned in https://github.com/palerdot/react-d3-speedometer/issues/153

Let us keep the documentation part as the last step. If everything is fine, we can do this as a last step.

PS: It is generally a good idea to create an issue, and discuss the proposed changes before making the change, as it might save time for both the contributor and the maintainer.

palerdot commented 1 year ago

We also need to consider what happens if the user (rightly) gives a negative value like -23. Will the text be automatically aligned at the top as expected? Please check on this and may post a screenshot of this scenario with this code changes.

Clarifying on this behaviour since it is configurable by the end user, if the value is positive, it should go below the gauge, if it is negative, it should go above the gauge. The text should not be rendered in the middle of the gauge (atleast for now). That is what I think, if this is tweakable by the end user. Otherwise, this prop will create whole new class of confusions and issues.

yoelbassin commented 1 year ago

What do you think about having this as some constant (that isn't configurable by the end user)? Again, my problem here is magically having 23 in two places in the code.

What do you think?

palerdot commented 1 year ago

What do you think about having this as some constant (that isn't configurable by the end user)? Again, my problem here is magically having 23 in two places in the code.

What do you think?

That works. Maybe declare VALUE_TEXT_Y_OFFSET somewhere in the config file and use it wherever needed. Introducing this prop will be confusing.