check on labels prop before computing base props for labels #584
Conversation
| if (labelText !== null && typeof labelText !== undefined || !events || !sharedEvents) { | ||
| const labelProp = props.labels || props[`${type}Labels`]; | ||
| if ( | ||
| labelText !== null && typeof labelText !== undefined || |
There was a problem hiding this comment.
BIG WARNING!!!
I know this is existing code, but we have a very big problem here:
> typeof labelText
< "undefined"
> typeof labelText !== undefined
< trueCan we create a high-priority ticket to scour all our victory repos for == undefined or something?
What we really want is:
> typeof labelText !== "undefined"
< falseThere was a problem hiding this comment.
Did a quick grep. I think the only other place it occurs is in this same file in master at: https://github.com/FormidableLabs/victory-chart/blob/master/src/components/victory-boxplot/helper-methods.js#L350
Side note -- we have lots of mixed foo === undefined and typeof foo === "undefined". Might make sense to create a cross-repo ticket to hone all of them to one approach (prefer the latter or _.isUndefined).
|
o.O yikes! I'll do that now. |
|
actually it looks like it was just in this one file (in victory-chart at least) |
| @@ -76,6 +76,7 @@ export default class App extends React.Component { | |||
| <VictoryChart style={chartStyle} domain={{ x: [0, 20], y: [0, 3] }}> | |||
| <VictoryBoxPlot | |||
| minLabels maxLabels | |||
| q1Labels={() => ""} | |||
There was a problem hiding this comment.
did you mean to commit this?
There was a problem hiding this comment.
This is a necessary change if you want labels to show up when you click on them. They need to be defined ahead of time even if just with a throwaway function.
| @@ -57,7 +57,7 @@ const getBaseProps = (props, fallbackProps) => { | |||
| }; | |||
| return data.reduce((childProps, datum, index) => { | |||
| const text = LabelHelpers.getText(props, datum, index); | |||
| if (text !== undefined && text !== null || events || sharedEvents) { | |||
| if (text !== undefined && text !== null || (labels && events || sharedEvents)) { | |||
There was a problem hiding this comment.
this is getting a little tough to read. many permutations. we could have separate variables, or group with parents, or comments
cc/ @chrisbolin
This PR checks on the existence of a
labelsprop (or corresponding prop) rather than calculating base props for labels any time there are events. This will be a breaking change for some small number of users, but the upgrade path is simple. Those impacted will just need to addlabels={() => null}or similar.