Fluid Width prop to take only necessary height about react-d3-speedometer

[palerdot]

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 #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.

[yoelbassin]

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]

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]

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]

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.