-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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][Monitor Management UI] Add Enabled column in monitor management monitors list table. #121682
[Uptime][Monitor Management UI] Add Enabled column in monitor management monitors list table. #121682
Conversation
Pinging @elastic/uptime (Team:uptime) |
5127c07
to
1cb17ad
Compare
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.
Looks great! Works like a charm. A few comments.
return ( | ||
<EuiSwitch | ||
checked={enabled} | ||
disabled={isLoading} |
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 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.
Sounds good.
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 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.
uptime-415-Enabled-progress-bar.mov
}); | ||
setRefresh(true); | ||
} | ||
}, [status]); // eslint-disable-line react-hooks/exhaustive-deps |
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.
Disabling exhaustive deps always gives me pause. Can you explain why you are disabling exhaustive deps?
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 here I want the side effect to execute only when status
changes. Without suppressing the linter, it would complain until isEnabled
is also provided to deps, along with others, which is not desired here.
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.
Thanks for the explanation. Could you add an inline comment for context?
} | ||
|
||
export const MonitorEnabled = ({ id, monitor, setRefresh }: Props) => { | ||
const [isEnabled, setIsEnabled] = useState<boolean | null>(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.
Can we set the default for this to monitor[ConfigKey.ENABLED]
.
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.
isEnabled
tracks the recent ui state. When it is null, default would be used. So I am doing const enabled = isEnabled ?? monitor[ConfigKey.ENABLED];
below.
checked={enabled} | ||
disabled={isLoading} | ||
showLabel={false} | ||
label={''} |
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 think you could get rid of the aria-label
below in favor of using the label here, paired with showLabel={false}
. I could be wrong.
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.
It does work indeed. Great to learn that, thanks.
x-pack/plugins/uptime/public/components/monitor_management/monitor_list/monitor_list.tsx
Outdated
Show resolved
Hide resolved
1cb17ad
to
791bdec
Compare
@elasticmachine merge upstream |
86561c2
to
60d5311
Compare
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
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Fixes elastic/uptime#415
Summary
Adds Enabled column to the monitor management monitors list table (manage_monitors page).
Screenshots
Videos
uptime-415-monitor-enabled-column.mov
Update
After adding progress bar to mark busy state of switch:
uptime-415-Enabled-progress-bar.mov
How to test this PR locally
Checklist
Delete any items that are not applicable to this PR.
[ ] Documentation was added for features that require explanation or tutorials[ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker listFor maintainers