-
Notifications
You must be signed in to change notification settings - Fork 88
check on labels prop before computing base props for labels #584
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,7 +44,7 @@ const getBaseProps = (props, fallbackProps) => { | |
props = assign({}, modifiedProps, getCalculatedValues(modifiedProps)); | ||
const { | ||
data, domain, events, groupComponent, height, interpolation, origin, padding, polar, | ||
scale, sharedEvents, standalone, style, theme, width | ||
scale, sharedEvents, standalone, style, theme, width, labels | ||
} = props; | ||
const initialChildProps = { | ||
parent: { | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is getting a little tough to read. many permutations. we could have separate variables, or group with parents, or comments |
||
const eventKey = datum.eventKey || index; | ||
childProps[eventKey] = { labels: LabelHelpers.getProps(props, index) }; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -367,7 +367,11 @@ const getBaseProps = (props, fallbackProps) => { | |
|
||
TYPES.forEach((type) => { | ||
const labelText = getText(dataProps, type); | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BIG WARNING!!! I know this is existing code, but we have a very big problem here: > typeof labelText
< "undefined"
> typeof labelText !== undefined
< true Can we create a high-priority ticket to scour all our victory repos for What we really want is: > typeof labelText !== "undefined"
< false There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did a quick grep. I think the only other place it occurs is in this same file in Side note -- we have lots of mixed |
||
labelProp && (events || sharedEvents) | ||
) { | ||
const target = `${type}Labels`; | ||
acc[eventKey][target] = getLabelProps(dataProps, labelText, type); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you mean to commit this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.