-
Notifications
You must be signed in to change notification settings - Fork 8
Global health bar with recharts #904
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development/1.0
Are you sure you want to change the base?
Global health bar with recharts #904
Conversation
Hello jeanmarcmilletscality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
ConflictThere is a conflict between your branch Please resolve the conflict on the feature branch ( git fetch && \
git checkout origin/feature/global-health-bar-recharts && \
git merge origin/development/1.0 Resolve merge conflicts and commit git push origin HEAD:feature/global-health-bar-recharts |
c6c8af7
to
73aebbd
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
9c35fde
to
5e1f951
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
1d9f433
to
14b943c
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
…ry slider, and utils with calc functions
…oltip functionality and data handling. Remove unused HistoryProvider and HistorySlider components. Update tooltip styling and data structure for alerts. Add 'Small' variant to text component
…actor data handling with useHealthBarData hook.
…XAxis component and consolidating utility constants in utils.tsx. Update CustomTooltip to accept tooltipProps for enhanced functionality.
…lobalHealthBar. Update GlobalHealthBar to utilize the new portal for tooltip display, enhancing positioning and visibility.
Add test
Add test for healthbar hook
14b943c
to
fcdaae5
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
startTimestamp: number, | ||
endTimestamp: number, | ||
) => { | ||
return (props: any, key: string) => { |
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.
return (props: any, key: string) => { | |
return (props: SHOULD_BE_RECT_PROPS, key: string) => { |
|
||
setTooltipPosition({ left, top }); | ||
} | ||
}, [coordinate, chartContainerRef]); |
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.
nit(for later): I feel like the whole logic of the effect also needs to be executed on scroll and window resize 🤔
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.
Probably some of this can be computed using utils from floating-ui instead of re-inventing the wheel 🤔
shouldShowLabel && ( | ||
<g transform={`translate(${payload.coordinate},${y})`}> | ||
<text | ||
textAnchor="middle" | ||
dy={10} | ||
dx={edgeMargin} | ||
fontSize={fontSize.smaller} | ||
fill={theme.textSecondary} | ||
> | ||
{is7DaySpan || isDaySpan ? ( | ||
<FormattedDateTime | ||
format="day-month-abbreviated-hour-minute" | ||
value={new Date(payload.value)} | ||
/> | ||
) : ( | ||
<FormattedDateTime format="time" value={new Date(payload.value)} /> | ||
)} | ||
</text> | ||
</g> |
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.
Isn't this the same logic for linetemporal chart and temporal bar charts ?
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.
There is no such logic in Barchart but we could use it in Line chart
const handlePointerEnter = useCallback( | ||
(key: string) => { | ||
setTooltipData(data[0][`alert_${key}`]); | ||
}, | ||
[data], | ||
); | ||
|
||
const handlePointerLeave = useCallback(() => { | ||
setTooltipData(null); | ||
}, []); |
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.
Shouldn't be only pointer, but focus changes so that keyboard navigation can trigger the tooltip
dataKey: string; | ||
yAxisId: string; | ||
fill: string; | ||
shape: (props: BarProps) => JSX.Element; |
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.
pretty sure we can replace this props by alertstartTime and endTime and simplify usage of this alertBar component
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.
@JBWatenbergScality Remove it as proper data for recharts make it useless
* Calculates rectangle properties for alert bars | ||
*/ | ||
export const getRectangleProps = ( | ||
props: any, |
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.
would be nice to type this
); | ||
|
||
const data = useMemo( | ||
() => [ |
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.
I'm not sure to understand why data needs to be an array if there is always a single entry in there
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.
Because Barchart expect an array for data.
I compute everything in data so I propose to separate the data needs for the barchart from the data for tooltip and bars
…hart and data for tooltip. Update related tests
Reworks Global health bar using Recharts lib
Tooltip:
Label:
Next Step: Add round edges to bars when bars is at start or end of chart (use mask?)