-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Uptime] Improve duration chart #58404
[Uptime] Improve duration chart #58404
Conversation
Pinging @elastic/uptime (Team:uptime) |
export const DurationChart: React.FC<Props> = ({ monitorId }: Props) => { | ||
const [getUrlParams] = useUrlParams(); | ||
const { dateRangeStart, dateRangeEnd } = getUrlParams(); | ||
|
||
const { monitor_duration, loading } = useSelector(selectDurationLines); | ||
|
||
const dispatch = useDispatch(); | ||
|
||
const { lastRefresh } = useContext(UptimeRefreshContext); | ||
|
||
useEffect(() => { | ||
dispatch( | ||
getMonitorDurationAction({ monitorId, dateStart: dateRangeStart, dateEnd: dateRangeEnd }) | ||
); | ||
}, [dateRangeStart, dateRangeEnd, dispatch, lastRefresh, monitorId]); | ||
|
||
return ( | ||
<DurationChartComponent | ||
locationDurationLines={monitor_duration?.locationDurationLines ?? []} | ||
loading={loading} | ||
/> | ||
); | ||
}; |
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.
@justinkambic using hooks redux becomes much cleaner and simple. At least the connect part.
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.
++ was just thinking that to myself. I have never used the hooks before; does this mean...
const { monitor_duration, loading } = useSelector(selectDurationLines);
...is effectively mapStateToProps
? And we could use multiple selectors if needed to get the state we want, instead of doing it in a function we'd pass to connect
?
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.
yes, you can use as many selectors as you like.
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 had a few comments for clarification or minor tweaks. There are also a number of comments directed more for me to create follow-up issues that are unrelated to the work you've done here.
This looks awesome and it's great to see so much code getting removed 👍
x-pack/legacy/plugins/uptime/public/components/connected/charts/monitor_duration.tsx
Show resolved
Hide resolved
x-pack/plugins/uptime/server/rest_api/monitors/monitors_durations.ts
Outdated
Show resolved
Hide resolved
@justinkambic took care of PR feedback. |
This looks good, want to get it mergeable and I will do a final functional test? |
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.
LGTM WFG
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…re/files-and-filetree * 'master' of github.com:elastic/kibana: (254 commits) Convert discover_page to ts, remove redundunt methods (elastic#59312) [Fix for Vis Editor] Revert setting time field to empty string when it's undefined (elastic#58873) Delete legacy search endpoint (elastic#59341) [Uptime] Improve duration chart (elastic#58404) [Snapshot & Restore] NP migration (elastic#59109) [ML] Add support for date_nanos time field in anomaly job wizard (elastic#59017) Revert "Makes alerting and actions optional properties for interface RequestH… (elastic#59264)" Change remote_clusters ID to remoteClusters (elastic#59246) Makes alerting and actions optional properties for interface RequestH… (elastic#59264) Clean up date histogram agg type. (elastic#58805) [ML] Management: fix license unsubscribe (elastic#59365) Remove documentation for server.cors settings (elastic#59096) Edit alert flyout (elastic#58964) [SIEM] Fix rule delete/duplicate actions (elastic#59306) move mouse to close obstructing tooltip (elastic#59214) Reset page after deleting (elastic#59310) Make sure phrases input filter triggers autosuggestons (elastic#59299) Add loading count source for http requests (elastic#59245) Revert "[ML] Transforms: Deprecate custom KibanaContext. (elastic#59133)" Expose metrics service to public API (elastic#59294) ... # Conflicts: # src/plugins/console/public/application/containers/editor/legacy/console_editor/editor.tsx
Friendly reminder: Looks like this PR hasn’t been backported yet. |
* use differential colors for duration chart * remove duration chart gql * update type * type fix * fix tyoe * update translation * update test * update conflicts * type checking * update snaps * PR feedback * PR feedback Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* use differential colors for duration chart * remove duration chart gql * update type * type fix * fix tyoe * update translation * update test * update conflicts * type checking * update snaps * PR feedback * PR feedback Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
Fix #58403
Remove only provided color and let the EUI chart coloring scheme work it out.
Also converted duration lines endpoint to rest api.