[Synthetics] Add missing monitorType and tag info in cards !!#188824
Merged
shahzad31 merged 11 commits intoelastic:mainfrom Jul 23, 2024
Merged
[Synthetics] Add missing monitorType and tag info in cards !!#188824shahzad31 merged 11 commits intoelastic:mainfrom
shahzad31 merged 11 commits intoelastic:mainfrom
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Contributor
Author
|
/ci |
Contributor
|
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
Member
|
@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? |
dominiqueclarke
requested changes
Jul 22, 2024
Contributor
dominiqueclarke
left a comment
There was a problem hiding this comment.
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: |
cauemarcondes
approved these changes
Jul 23, 2024
Contributor
cauemarcondes
left a comment
There was a problem hiding this comment.
LGTM, just some nits
Comment on lines
+23
to
+47
| <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> |
Contributor
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
i tried it doesn't work with onClickAriaLabel being present always. it's kinda weird.
Comment on lines
+17
to
+37
| 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 && ( |
Contributor
There was a problem hiding this comment.
Suggested change
| 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
TinLe
added a commit
to TinLe/kibana
that referenced
this pull request
Jul 30, 2024
* 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) ...
TinLe
added a commit
to TinLe/kibana
that referenced
this pull request
Jul 30, 2024
* 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) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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