-
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
[Synthetics] Add missing monitorType and tag info in cards !! #188824
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
/ci |
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
@shahzad31 Can you please highlight what has changed in the before and after images? It feels like a find the difference game :D Also, would you please share how should we test this PR? |
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.
When there are many tags and they are uncollapsed, the UI appears broken.
Also, the monitor type tag is not keyboard accessible, and needs to be since it can be used to apply a filter to the page.
See how the tags themselves are keyboard accessible since they are kickable.
https://www.loom.com/share/d37ebaaf53dd4c3783d9d2f7659e481b?sid=e0b330bb-4976-4479-af02-8aafc0995382
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
History
To update your PR or re-run it, just comment with: |
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, just some nits
<EuiBadge | ||
onClick={onClick} | ||
onClickAriaLabel={getFilterTitle(monitorType)} | ||
title={ariaLabel} | ||
aria-label={ariaLabel} | ||
iconType={getMonitorTypeBadgeIcon(monitorType)} | ||
onMouseDown={(e: MouseEvent) => { | ||
// Prevents the click event from being propagated to the @elastic/chart metric | ||
e.stopPropagation(); | ||
}} | ||
> | ||
{getMonitorTypeBadgeTitle(monitorType)} | ||
</EuiBadge> | ||
) : ( | ||
badge | ||
<EuiBadge | ||
title={ariaLabel} | ||
aria-label={ariaLabel} | ||
iconType={getMonitorTypeBadgeIcon(monitorType)} | ||
onMouseDown={(e: MouseEvent) => { | ||
// Prevents the click event from being propagated to the @elastic/chart metric | ||
e.stopPropagation(); | ||
}} | ||
> | ||
{getMonitorTypeBadgeTitle(monitorType)} | ||
</EuiBadge> |
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.
You don't need to check if onClick
is defined and have two variants for this component. If onClick
is undefined it'll pass undefined to the EuiBadge
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.
i tried it doesn't work with onClickAriaLabel being present always. it's kinda weird.
const tags = monitor.tags; | ||
const history = useHistory(); | ||
|
||
return ( | ||
<> | ||
<EuiSpacer size="xs" /> | ||
<EuiFlexGroup gutterSize="xs"> | ||
<EuiFlexItem grow={false}> | ||
<MonitorTypeBadge | ||
monitorType={monitor[ConfigKey.MONITOR_TYPE]} | ||
ariaLabel={labels.getFilterForTypeMessage(monitor[ConfigKey.MONITOR_TYPE])} | ||
onClick={() => { | ||
history.push({ | ||
search: `monitorTypes=${encodeURIComponent( | ||
JSON.stringify([monitor[ConfigKey.MONITOR_TYPE]]) | ||
)}`, | ||
}); | ||
}} | ||
/> | ||
</EuiFlexItem> | ||
{(tags ?? []).length > 0 && ( |
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.
const tags = monitor.tags; | |
const history = useHistory(); | |
return ( | |
<> | |
<EuiSpacer size="xs" /> | |
<EuiFlexGroup gutterSize="xs"> | |
<EuiFlexItem grow={false}> | |
<MonitorTypeBadge | |
monitorType={monitor[ConfigKey.MONITOR_TYPE]} | |
ariaLabel={labels.getFilterForTypeMessage(monitor[ConfigKey.MONITOR_TYPE])} | |
onClick={() => { | |
history.push({ | |
search: `monitorTypes=${encodeURIComponent( | |
JSON.stringify([monitor[ConfigKey.MONITOR_TYPE]]) | |
)}`, | |
}); | |
}} | |
/> | |
</EuiFlexItem> | |
{(tags ?? []).length > 0 && ( | |
const tags = monitor.tags ?? []; | |
const history = useHistory(); | |
return ( | |
<> | |
<EuiSpacer size="xs" /> | |
<EuiFlexGroup gutterSize="xs"> | |
<EuiFlexItem grow={false}> | |
<MonitorTypeBadge | |
monitorType={monitor[ConfigKey.MONITOR_TYPE]} | |
ariaLabel={labels.getFilterForTypeMessage(monitor[ConfigKey.MONITOR_TYPE])} | |
onClick={() => { | |
history.push({ | |
search: `monitorTypes=${encodeURIComponent( | |
JSON.stringify([monitor[ConfigKey.MONITOR_TYPE]]) | |
)}`, | |
}); | |
}} | |
/> | |
</EuiFlexItem> | |
{tags.length > 0 && ( |
.../apps/synthetics/components/monitors_page/overview/overview/metric_item/metric_item_body.tsx
Show resolved
Hide resolved
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
* master: (3487 commits) `BedrockChat` & `GeminiChat` (elastic#186809) [ResponseOps] log error when ES Query rules find docs out of time range (elastic#186332) skip flaky suite (elastic#188997) [Security solution][Alert Details] Enable preview feature flag and cypress tests (elastic#188580) [EuiProviders] Warn Developer if EuiProvider is missing (elastic#184608) [Security Solution ] Fixes Timeline infinite loading bug (elastic#188943) Improve SearchSource SearchRequest type (elastic#186862) Deprecate Search Sessions config (elastic#188037) [Synthetics] Add missing monitorType and tag info in cards !! (elastic#188824) [Console Monaco] Resolve uncaught error from tokenizer (elastic#188746) [Data Forge] Add `service.logs` dataset as a data stream (elastic#188786) [Console] Fix failing bulk requests (elastic#188552) Update dependency terser to ^5.31.2 (main) (elastic#188528) [APM][ECO] Telemetry (elastic#188627) [Fleet] Fix uninstall package validation accross space (elastic#188749) Update warning on `xpack.fleet.enableExperimental` (elastic#188917) [DOCS][Cases] Automate more screenshots for cases (elastic#188697) [Fleet] Fix get one agent when feature flag disabled (elastic#188953) chore(investigate): Add investigate-app plugin from poc (elastic#188122) [Monaco Editor] Add Search functionality (elastic#188337) ...
* master: (2400 commits) `BedrockChat` & `GeminiChat` (elastic#186809) [ResponseOps] log error when ES Query rules find docs out of time range (elastic#186332) skip flaky suite (elastic#188997) [Security solution][Alert Details] Enable preview feature flag and cypress tests (elastic#188580) [EuiProviders] Warn Developer if EuiProvider is missing (elastic#184608) [Security Solution ] Fixes Timeline infinite loading bug (elastic#188943) Improve SearchSource SearchRequest type (elastic#186862) Deprecate Search Sessions config (elastic#188037) [Synthetics] Add missing monitorType and tag info in cards !! (elastic#188824) [Console Monaco] Resolve uncaught error from tokenizer (elastic#188746) [Data Forge] Add `service.logs` dataset as a data stream (elastic#188786) [Console] Fix failing bulk requests (elastic#188552) Update dependency terser to ^5.31.2 (main) (elastic#188528) [APM][ECO] Telemetry (elastic#188627) [Fleet] Fix uninstall package validation accross space (elastic#188749) Update warning on `xpack.fleet.enableExperimental` (elastic#188917) [DOCS][Cases] Automate more screenshots for cases (elastic#188697) [Fleet] Fix get one agent when feature flag disabled (elastic#188953) chore(investigate): Add investigate-app plugin from poc (elastic#188122) [Monaco Editor] Add Search functionality (elastic#188337) ...
Summary
Add missing monitorType and tag info in cards !!
Testing
Easiest way to test is connect to an oblt cluster and go to synthetics UI to create few monitors
After
Before
Highlighted change