[Console] Progress bar #56628

jloleysens · 2020-02-03T14:48:23Z

Summary

Fix #24157

Gif

How to review code

Review the new network status bar component
Review SCSS, currently the network status bar is rendered inside of the editor output component and relatively positioned just outside of the output pane. We could place next to the History and Settings controls but this causes the underline to not stretch the full width of the editor.

How to review UX

Start Kibana via yarn start with an ES node running (e.g., yarn es snapshot)
If in chrome, open Dev tools, switch to the network tab, expand the "Online" dropdown and simulate a slow 3G connection.
Run a request

Release note

We also enhanced the Console UI with a spinner and more information about how a network request to ES went.

Final Gif

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
~~Any text added follows EUI's writing guidelines, uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md]~~
~~Documentation was added for features that require explanation or tutorials~~
~~Unit or functional tests were updated or added to match the most common scenarios~~
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

- progress bar loader for in flight request - Network request status bar

elasticmachine · 2020-02-03T14:49:13Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

jloleysens · 2020-02-04T09:35:46Z

@elasticmachine merge upstream

cjcenizal

This looks fantastic @jloleysens! I like the UI and UX decisions you made here. I had a few suggestions and questions, but the only thing I'd like to block on is my concern about the UX regarding sending a request while there's already a request in flight.

cjcenizal · 2020-02-04T21:13:14Z

src/plugins/console/public/application/containers/editor/editor.tsx

-    </PanelsContainer>
+    <>
+      {requestInFlight ? (
+        <div style={{ position: 'relative', zIndex: 2000 }}>


Can we use a class name here instead? I'm not sure if our CSP disallows inline styles, but I think we are moving in that direction.

That's an interesting point! Do you know whether this is the current CSP src/core/server/csp/config.ts?

Oh cool, didn't know about that file! I'm not sure but it looks like it would be.

cjcenizal · 2020-02-04T21:17:09Z

...legacy/core_plugins/console_legacy/public/styles/components/_network_request_status_bar.scss

@@ -0,0 +1,5 @@
+.conApp__outputNetworkRequestStatusBar {


Can we use an EuiFlexGroup to handle layout instead of custom classes?

Originally, I did use an EuiFlexGroup, but found the padding on bottom and top too big. Given your suggested changes below I am happy to change this!

cjcenizal · 2020-02-04T21:18:59Z

...sole/public/application/components/network_request_status_bar/network_request_status_bar.tsx

+  >
+    <EuiFlexItem
+      grow={false}
+      className="conApp__outputNetworkRequestStatusBar__item conApp__outputNetworkRequestStatusBar__badge"


Is conApp__outputNetworkRequestStatusBar__badge defined anywhere?

Also if we remove the conApp__outputNetworkRequestStatusBar__item class and set gutterSize="s" on the EuiFlexGroup on line 63, we'll get this layout which I think looks a bit better:

Will clean that up!

cjcenizal · 2020-02-04T21:38:58Z

src/plugins/console/public/lib/es/es.ts

      if (jqXHR.status === 0) {
        jqXHR.responseText =
          "\n\nFailed to connect to Console's backend.\nPlease check the Kibana server is up and running";
      }
-      wrappedDfd.rejectWith(this, [jqXHR, textStatus, errorThrown]);
-    }
+      wrappedDfd.rejectWith({}, [jqXHR, textStatus, errorThrown]);


I'm trying to understand why this file was changed. Was this line the only material change?

Mainly it was convernted to .ts. That particular change is for the this object of the callback handler - which should actually not be anything.

cjcenizal · 2020-02-04T21:46:12Z

...plugins/console/public/application/containers/editor/legacy/console_editor/editor_output.tsx

+    <>
+      {lastDatum ? (
+        <div className="conApp__networkRequestContainer">
+          <NetworkRequestStatusBar


It looks like the location of this component causes some rendering problems when you open the "History" dropdown:

In terms of component hierarchy, I'd also expect to find it located near the tabs in the code, since it's located near the tabs visually. If we change https://github.com/elastic/kibana/blob/master/src/plugins/console/public/application/containers/main/main.tsx#L70 to this:

<EuiFlexItem grow={false}> <EuiTitle className="euiScreenReaderOnly"> <h1> {i18n.translate('console.pageHeading', { defaultMessage: 'Console', })} </h1> </EuiTitle> <EuiFlexGroup gutterSize="none"> <EuiFlexItem grow={false}> <TopNavMenu disabled={!done} items={getTopNavConfig({ onClickHistory: () => setShowHistory(!showingHistory), onClickSettings: () => setShowSettings(true), onClickHelp: () => setShowHelp(!showHelp), })} /> </EuiFlexItem> <EuiFlexItem className="conApp__tabsExtension"> <NetworkRequestStatusBar method={'GET'} endpoint={'/the_last_starfighter'} statusCode={200} statusText={':)'} timeElapsedMs={'411'} /> </EuiFlexItem> </EuiFlexGroup> </EuiFlexItem>

And add this class somewhere:

.conApp__tabsExtension { border-bottom: $euiBorderThin; }

Then the rendered output is correct and the two components will be colocated:

Awesome! Happy with these changes, I was a bit stuck on this one 👍

cjcenizal · 2020-02-04T21:50:56Z

...sole/public/application/components/network_request_status_bar/network_request_status_bar.tsx

+          }`}</EuiText>
+        }
+      >
+        <EuiBadge color={mapStatusCodeToBadgeColor(statusCode)}>


What do you think of hiding the second badge and replacing this one with a hollow one that says "Request in flight" while the request is in flight? I think it improves usability by 1) acting as a placeholder for the request results so you know where to look for them and 2) explaining the purpose of the strobing progress indicator.

cjcenizal · 2020-02-04T21:57:25Z

...le/public/application/hooks/use_send_current_request_to_es/use_send_current_request_to_es.ts

  const dispatch = useRequestActionContext();

  return useCallback(async () => {
+    if (requestInFlight) {


I might be missing something, but I think this condition introduces a UX problem. Current behavior in master is that the latest request supersedes older in-flight requests. So, if you have a long-running request and then dispatch a new request, it will appear as if the long-running request was cancelled because the new request's response will appear instead. This is useful in case you get frustrated with a long-running request and want to try something different.

With the change made here, the user is forced to sit through a long-running request and has to reload Console to "cancel" it. Was there another problem this change was intended to fix?

You are totally correct, this is an artefact that should have been removed!!

cjcenizal · 2020-02-04T22:31:17Z

...le/public/application/hooks/use_send_current_request_to_es/use_send_current_request_to_es.ts

    dispatch({ type: 'sendRequest', payload: undefined });
    try {
      const editor = registry.getInputEditor();
      const requests = await editor.getRequestsInRange();
      if (!requests.length) {
        dispatch({
          type: 'requestFail',
-          payload: { value: 'No requests in range', contentType: 'text/plain' },
+          payload: {


I feel like the way this is represented in the UI isn't particularly useful. I didn't actually send a request to an endpoint, and there was no real response nor elapsed time, so this information in the top-right corner doesn't seem relevant:

Can we explore ways to make this more useful? How about we preserve the existing information from the prior request (i.e. the response payload in the output pane as well as the status and elapsed time badges), and instead just show a neutral toast to let the user know that they need to move their cursor inside of a request or highlight multiple requests in order to execute them?

- Clean up unused class names - Move the network request status bar to next to the nav bar with a grey line under it - Added the request in progress badge in the network request status bar - Removed logic forcing one request at a time! - Added notification for when a user is sending a request from a line with no request on it (no more 0 - None status). Also preserve the previuous request in this case

…ext area!! This can be backported to 7.6 as it causes when a request is selected at the bottom of a large text buffer and the history viewer is opened.

elizabetdev

@jloleysens the code looks great. I agree with @cjceniza, you made very good design decisions! 🎉

elizabetdev · 2020-02-05T14:04:28Z

src/legacy/core_plugins/console_legacy/public/styles/_app.scss

+  position: absolute;
+  min-width: 200px;
+  width: 100%;
+


I would just remove this empty space.

elizabetdev · 2020-02-05T14:04:51Z

src/legacy/core_plugins/console_legacy/public/styles/_app.scss

+
+  height: 30px;
+  top: -30px;
+


I would also remove this empty space.

cjcenizal

Looks great! Had one copy suggestion.

cjcenizal · 2020-02-05T15:14:50Z

...le/public/application/hooks/use_send_current_request_to_es/use_send_current_request_to_es.ts

+        notifications.toasts.add(
+          i18n.translate('console.notification.noReqeustSelectedTitle', {
+            defaultMessage:
+              'It looks like your cursor is not on a request. Please select a request by placing your cursor on it.',


Nit: I think our copywriting guide discourages phrases like "it looks like" and niceties like "please". My suggestion is: "No request selected. Select a request by placing the cursor inside it."

Remove unused SCSS

cjcenizal · 2020-02-05T20:22:05Z

@elasticmachine merge upstream

elizabetdev

LGTM! 🎉

* First round of UI updates - progress bar loader for in flight request - Network request status bar * Add notification about in flight request * Use nbsp; and update the EuiCode to use EuiBadge * Address PR feedback: - Clean up unused class names - Move the network request status bar to next to the nav bar with a grey line under it - Added the request in progress badge in the network request status bar - Removed logic forcing one request at a time! - Added notification for when a user is sending a request from a line with no request on it (no more 0 - None status). Also preserve the previuous request in this case * [NB] Fix for floating tools when request is past bottom of rendered text area!! This can be backported to 7.6 as it causes when a request is selected at the bottom of a large text buffer and the history viewer is opened. * Fix types * Update copy Remove unused SCSS Co-authored-by: Elastic Machine <[email protected]>

kibanamachine · 2020-02-06T09:28:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ff24cc6

History

💚 Build #24719 succeeded ff24cc6
💔 Build #24706 failed a3c685e
💚 Build #24549 succeeded 7c9390c
💔 Build #24540 failed 24dd028
💚 Build #24253 succeeded 191146f

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…ix] (#56948) * [NB] Fix for floating tools when request is past bottom of rendered text area!! This can be backported to 7.6 as it causes when a request is selected at the bottom of a large text buffer and the history viewer is opened. * 'unset' -> 'auto'

* [Console] Progress bar (#56628) * First round of UI updates - progress bar loader for in flight request - Network request status bar * Add notification about in flight request * Use nbsp; and update the EuiCode to use EuiBadge * Address PR feedback: - Clean up unused class names - Move the network request status bar to next to the nav bar with a grey line under it - Added the request in progress badge in the network request status bar - Removed logic forcing one request at a time! - Added notification for when a user is sending a request from a line with no request on it (no more 0 - None status). Also preserve the previuous request in this case * [NB] Fix for floating tools when request is past bottom of rendered text area!! This can be backported to 7.6 as it causes when a request is selected at the bottom of a large text buffer and the history viewer is opened. * Fix types * Update copy Remove unused SCSS Co-authored-by: Elastic Machine <[email protected]> * 'unset' -> 'auto' Co-authored-by: Elastic Machine <[email protected]>

jloleysens added 3 commits February 1, 2020 18:28

First round of UI updates

c6da65c

- progress bar loader for in flight request - Network request status bar

Add notification about in flight request

799a003

Use nbsp; and update the EuiCode to use EuiBadge

f65381a

jloleysens requested a review from cjcenizal February 3, 2020 14:48

jloleysens requested a review from a team as a code owner February 3, 2020 14:48

jloleysens added Feature:Console Dev Tools Console Feature Feature:Dev Tools release_note:enhancement v7.7.0 v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more labels Feb 3, 2020

This was referenced Feb 3, 2020

GUI improvement: progress bar in Kibana's console #45810

Closed

[Console] Repositioning console controls #56653

Closed

elizabetdev self-requested a review February 3, 2020 18:29

Merge branch 'master' into feature/console/ui-progress-bar

191146f

cjcenizal suggested changes Feb 4, 2020

View reviewed changes

jloleysens added 3 commits February 5, 2020 11:48

[NB] Fix for floating tools when request is past bottom of rendered t…

24dd028

…ext area!! This can be backported to 7.6 as it causes when a request is selected at the bottom of a large text buffer and the history viewer is opened.

Fix types

7c9390c

elizabetdev suggested changes Feb 5, 2020

View reviewed changes

cjcenizal approved these changes Feb 5, 2020

View reviewed changes

Update copy

a3c685e

Remove unused SCSS

Merge branch 'master' into feature/console/ui-progress-bar

ff24cc6

elizabetdev approved these changes Feb 5, 2020

View reviewed changes

jloleysens merged commit d20a7d0 into elastic:master Feb 6, 2020

jloleysens deleted the feature/console/ui-progress-bar branch February 6, 2020 07:47

jloleysens mentioned this pull request Feb 6, 2020

[7.x] [Console] Progress bar (#56628) #56946

Merged

jloleysens mentioned this pull request Feb 6, 2020

[7.6] [Console] Editor with open History fix (#56628) [Non-blocking fix] #56948

Merged

cjcenizal mentioned this pull request Mar 11, 2020

Enhancement Request: Show HTTP Response Code in Dev Tools #15288

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Progress bar #56628

[Console] Progress bar #56628

jloleysens commented Feb 3, 2020 •

edited

Loading

elasticmachine commented Feb 3, 2020

jloleysens commented Feb 4, 2020

cjcenizal left a comment

cjcenizal Feb 4, 2020

jloleysens Feb 5, 2020

cjcenizal Feb 5, 2020

cjcenizal Feb 4, 2020 •

edited

Loading

jloleysens Feb 5, 2020

cjcenizal Feb 4, 2020 •

edited

Loading

jloleysens Feb 5, 2020

cjcenizal Feb 4, 2020

jloleysens Feb 5, 2020

cjcenizal Feb 5, 2020

cjcenizal Feb 4, 2020 •

edited

Loading

jloleysens Feb 5, 2020

cjcenizal Feb 4, 2020 •

edited

Loading

cjcenizal Feb 4, 2020 •

edited

Loading

jloleysens Feb 5, 2020

cjcenizal Feb 4, 2020 •

edited

Loading

elizabetdev left a comment

elizabetdev Feb 5, 2020

elizabetdev Feb 5, 2020

cjcenizal left a comment

cjcenizal Feb 5, 2020

cjcenizal commented Feb 5, 2020

elizabetdev left a comment

kibanamachine commented Feb 6, 2020

[Console] Progress bar #56628

[Console] Progress bar #56628

Conversation

jloleysens commented Feb 3, 2020 • edited Loading

Summary

How to review code

How to review UX

Release note

Checklist

For maintainers

elasticmachine commented Feb 3, 2020

jloleysens commented Feb 4, 2020

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

cjcenizal Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

elizabetdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Feb 5, 2020

elizabetdev left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 6, 2020

💚 Build Succeeded

History

jloleysens commented Feb 3, 2020 •

edited

Loading

cjcenizal Feb 4, 2020 •

edited

Loading

cjcenizal Feb 4, 2020 •

edited

Loading

cjcenizal Feb 4, 2020 •

edited

Loading

cjcenizal Feb 4, 2020 •

edited

Loading

cjcenizal Feb 4, 2020 •

edited

Loading

cjcenizal Feb 4, 2020 •

edited

Loading