Charts - add legendAllowWrap callback #8706
Conversation
|
Preview: https://patternfly-react-pr-8706.surge.sh A11y report: https://patternfly-react-pr-8706-a11y.surge.sh |
dlabrecq
force-pushed
the
v5-8643-legendAllowWrapCallback
branch
2 times, most recently
from
e4bd433 to
97090c7
Compare
dlabrecq
changed the title
dlabrecq
force-pushed
the
v5-8643-legendAllowWrapCallback
branch
from
97090c7 to
ed16f7f
Compare
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
Looks like these changes are causing github to highlight some lint errors - visible in this PR.
they read like
React Hook useEffect has missing dependencies: 'getLegend', 'legendAllowWrap', 'legendAllowWrapCallback', and 'theme'. Either include them or remove the dependency array. If 'legendAllowWrapCallback' changes too often, find the parent component that defines it and wrap that definition in useCallback
I think this implies there are a few best practices being violated (and we have issues open in the react backlog to update the linting in the pf-react project so hopefully we will catch these violations ourself in the near future.
|
@nicolethoen I've fixed the lint warnings. |
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
LGTM, just have one non blocking thought/question about the API.
| legendAllowWrap?: boolean; | ||
| /** | ||
| * @beta If legendAllowWrap is true, this function will be called after the legend's itemsPerRow property has been | ||
| * calculated, based on available width. The value provided can be used to increase the chart's parent container | ||
| * height as legend items wrap onto the next line. If no adjustment is necessary, the value will be zero. | ||
| */ | ||
| legendAllowWrapCallback?: (extraHeight: number) => void; |
There was a problem hiding this comment.
What would you think about updating the type for legendAllowWrap to be something like boolean | (extraHeight: number) => void; rather than adding a new prop explicitly for the CB that doesn't apply unless legendAllowWrap is true?
There was a problem hiding this comment.
@wise-king-sullyman I merged legendAllowWrap and legendAllowWrapCallback.
dlabrecq
force-pushed
the
v5-8643-legendAllowWrapCallback
branch
from
3879b7d to
99e927f
Compare
dlabrecq
force-pushed
the
v5-8643-legendAllowWrapCallback
branch
from
99e927f to
de08b23
Compare
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
5 participants
Added an optional callback to compliment the chart's
legendAllowWrapproperty. ThelegendAllowWrapCallbackprop will be used to adjust the chart's parent containerheightas legend items wrap in response to the availablewidth.Example:
Closes #8643