-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Console] Improve UX around handling multiple requests #132494
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
[Console] Improve UX around handling multiple requests #132494
Conversation
…-ref HEAD~1..HEAD --fix'
src/plugins/console/public/application/models/legacy_core_editor/mode/output_highlight_rules.js
Outdated
Show resolved
Hide resolved
...plugins/console/public/application/containers/editor/legacy/console_editor/editor_output.tsx
Outdated
Show resolved
Hide resolved
src/plugins/console/public/application/models/legacy_core_editor/mode/output_highlight_rules.js
Outdated
Show resolved
Hide resolved
...plugins/console/public/application/containers/editor/legacy/console_editor/editor_output.tsx
Outdated
Show resolved
Hide resolved
...sole/public/application/components/network_request_status_bar/network_request_status_bar.tsx
Outdated
Show resolved
Hide resolved
src/plugins/console/public/application/hooks/use_send_current_request/send_request.ts
Outdated
Show resolved
Hide resolved
src/plugins/console/public/application/hooks/use_send_current_request/send_request.ts
Outdated
Show resolved
Hide resolved
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
Verified functional tests through flaky-test-suite-runner https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/670 |
|
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
cjcenizal
left a comment
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.
@elastic/kibana-design Would you or some folks from EUI please review the styles here? I want to make sure they're correct, especially for dark mode.
@mibragimov People are going to love this enhancement! Thank you so much for making this change. I had one suggestion to add some more tests, other than that this code LGTM.
Did a quick test locally and things look good there too:
src/plugins/console/public/application/models/legacy_core_editor/mode/output_highlight_rules.ts
Outdated
Show resolved
Hide resolved
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
expected head sha didn’t match current head ref. |
ping @elastic/kibana-design |
MichaelMarcialis
left a comment
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've left a few style comments below for your review. Thanks!
| background-color: $euiColorSuccess; | ||
| color: chooseLightOrDarkText($euiColorSuccess); |
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.
Suggesting change to the correct badge background colors here.
| background-color: $euiColorSuccess; | |
| color: chooseLightOrDarkText($euiColorSuccess); | |
| background-color: $euiColorVis0_behindText; | |
| color: chooseLightOrDarkText($euiColorVis0_behindText); |
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.
@MichaelMarcialis For our own edification, can you teach us about the role of $euiColorVis0_behindText and similar colors, and what makes it better than $euiColorSuccess 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.
Good question, @cjcenizal! For the purposes of this PR, my reason for the suggested color changes were purely to make these ace badges consistent with the colors that our EuiBadge components use.
That said, I imagine the reason the EUI team ultimately chose to use the behind-text visualization colors for the EuiBadge component has to do with accessibility and the contrast ratio, as this component houses fairly small sized text. The contrast with the behind-text visualization colors appears to be higher and receives a better WCAG rating. Unlike EuiBadge, I assume that components with larger text sizes (such as EuiButton) can afford be a bit less strict with the color contrast, which is why they can use colors like $euiColorSuccess.
CCing @cchaos here to keep me honest and double-check my assumptions.
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.
For full context on why we used visualization colors for badges, you can read through this PR: elastic/eui#2455
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.
Thank you both! This helps me out a ton.
|
@elasticmachine merge upstream |
|
@MichaelMarcialis thanks for the review! I've addressed your feedback with ad57319, do you mind having another look? |
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 making those changes @mibragimov. I only have one additional thought. Would it be worth changing the font family of .ace_badge to $euiFontFamily in this case? Currently, the .ace_badges are inheriting the editor font family, which differentiates itself from the standard EUI badges. Assuming these badges only ever appear at the end of editor lines, I think this might look nicer/more consistent.
Otherwise, this looks good from my perspective. Approving now so that I don't hold you up further. Thanks.
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @mibragimov |

Fixes #130982
This PR adds support for highlighting the results for multiple requests in the editor output