Rich History: UX adjustments and fixes #22729

ivanahuckova · 2020-03-11T16:00:47Z

This PR adds fixes to Rich history based on users feedback. See list of fixes below:

Fixed:

Separate PRs:

Add documentation PR merged
Add test coverage open PR ready to be reviewed
UX fixes (based on user feedback) open PR
Add error handling for store
Add method "getQueryDisplayText" to most used datasources (elastic, graphite)
Sync open query history tabs when opened in different windows (always update after change) ->
Discussed with Andrej. We are probably going to look at this later (when we move history to database). Needs more research/thought regarding implementation.

ivanahuckova · 2020-03-13T10:18:28Z

public/app/core/store.ts

      return false;
    }
    try {
      this.set(key, json);
    } catch (error) {
      // Likely hitting storage quota
-      console.error(`Could not save item in localStorage: ${key}. [${error}]`);
+      appEvents.emit(AppEvents.alertError, [`Could not save item in localStorage: ${key}. [${error}]`]);
      return false;
    }
    return true;


Add error handling. Has to be done here, as store is not throwing errors and therefore if we add try-catch block to addQueryToRichHistory it is not going to work.

I would still like to lift this one level up. So I would rather have this function throw the error, handle it in the both the RichHistory helpers and in LocalStorageValueProvider where this is used. That way LocalStorageValueProvider can still keep the console.log as we don't know if the emit is appropriate there, and add the emit to rich history where we know we want it. We can do it in separate PR.

ivanahuckova · 2020-03-13T10:20:50Z

public/app/core/utils/richHistory.ts

-  activeDatasourceOnly: `${RICH_HISTORY_KEY}.activeDatasourceOnly`,
+  retentionPeriod: 'grafana.explore.richHistory.retentionPeriod',
+  starredTabAsFirstTab: 'grafana.explore.richHistory.starredTabAsFirstTab',
+  activeDatasourceOnly: 'grafana.explore.richHistory.activeDatasourceOnly',
 };


Remove interpolation and stitching of keys for easier search-ability.

ivanahuckova · 2020-03-13T10:22:12Z

public/app/core/utils/richHistory.ts

+      return newHistory;
+    } else {
+      return richHistory;
+    }


Error handling - if object is not saved in store, it returns false. Therefore we need to return newHistory if saving was successful and previous history if it wasn't successful.

ivanahuckova · 2020-03-13T10:22:30Z

public/app/features/explore/Explore.tsx

@@ -322,7 +322,6 @@ export class Explore extends React.PureComponent<ExploreProps, ExploreState> {
                  ['explore-active-button']: showRichHistory,
                })}
                onClick={this.toggleShowRichHistory}
-                disabled={isLive}


If live-tailing is active, don't disable button to open and close Rich history.

ivanahuckova · 2020-03-13T10:23:37Z

public/app/features/explore/Explore.tsx

+                          width={width}
+                          exploreId={exploreId}
+                          onClose={this.toggleShowRichHistory}
+                        />


Pass function toggleShowRichHistory, so we can add closing button to Rich history drawer.

ivanahuckova · 2020-03-13T10:24:57Z

public/app/features/explore/RichHistory/RichHistory.tsx

  deleteRichHistory: () => void;
+  onClose: () => void;
 }


New props:

height 0 so the slider can adjust its height based on height of drawer.

onClose - to close rich history drawer by new x button

ivanahuckova · 2020-03-13T10:25:27Z

public/app/features/explore/RichHistory/RichHistory.tsx

@@ -166,6 +166,7 @@ class UnThemedRichHistory extends PureComponent<RichHistoryProps, RichHistorySta
          onChangeSortOrder={this.onChangeSortOrder}
          onSelectDatasourceFilters={this.onSelectDatasourceFilters}
          exploreId={exploreId}
+          height={height}
        />


To adjust slider's height

ivanahuckova · 2020-03-13T10:25:51Z

public/app/features/explore/RichHistory/RichHistory.tsx

@@ -205,8 +206,7 @@ class UnThemedRichHistory extends PureComponent<RichHistoryProps, RichHistorySta
      icon: 'gicon gicon-preferences',
    };

-    let tabs = starredTabAsFirstTab ? [StarredTab, QueriesTab, SettingsTab] : [QueriesTab, StarredTab, SettingsTab];
-
+    let tabs = [QueriesTab, StarredTab, SettingsTab];
    return (


Remove switching of tab bars based on active tab.

ivanahuckova · 2020-03-13T10:27:19Z

public/app/features/explore/RichHistory/RichHistoryCard.tsx

+              }}
+              onClick={e => {
+                e.stopPropagation();
+              }}


Add stop propagation, as clicking on comment and comment buttons also triggered adding queries to query fields.

This seems weird is it a bug in the form elements?

ivanahuckova · 2020-03-13T10:27:32Z

public/app/features/explore/RichHistory/RichHistoryContainer.tsx

  const handleDots = theme.isLight ? theme.colors.gray70 : theme.colors.gray33;
-  const handleDotsHover = theme.isLight ? theme.colors.gray33 : theme.colors.dark7;
+  const handleBackgroundHover = theme.isLight ? theme.colors.gray70 : theme.colors.gray33;
+  const handleDotsHover = theme.isLight ? theme.colors.gray5 : theme.colors.dark7;



More semantic naming.

ivanahuckova · 2020-03-13T10:28:17Z

public/app/features/explore/RichHistory/RichHistoryContainer.tsx

@@ -118,13 +120,18 @@ function RichHistoryContainer(props: Props) {
      maxHeight="100vh"
      maxWidth={drawerWidth}
      minWidth={drawerWidth}
+      onResize={(e, dir, ref) => {
+        setHeight(Number(ref.style.height.slice(0, -2)));
+      }}
    >


On drawer resize, get the height of the drawer. It is only stored in style.

ivanahuckova · 2020-03-13T10:29:12Z

public/app/features/explore/RichHistory/RichHistorySettings.tsx

+      },
+    });
+  };
+


Add Are you sure you want to permanently delete your query history modal before deleting history.

ivanahuckova · 2020-03-13T10:29:54Z

public/app/features/explore/RichHistory/RichHistorySettings.tsx

-          appEvents.emit(AppEvents.alertSuccess, ['Query history deleted']);
-        }}
-      >
+      <Forms.Button variant="destructive" onClick={onDelete}>
        Clear query history


Add notification that history was successfully deleted.

aocenas · 2020-03-13T10:39:09Z

public/app/core/store.ts

      return false;
    }
    try {
      this.set(key, json);
    } catch (error) {
      // Likely hitting storage quota
-      console.error(`Could not save item in localStorage: ${key}. [${error}]`);
+      appEvents.emit(AppEvents.alertError, [`Could not save item in localStorage: ${key}. [${error}]`]);
      return false;
    }
    return true;


I would still like to lift this one level up. So I would rather have this function throw the error, handle it in the both the RichHistory helpers and in LocalStorageValueProvider where this is used. That way LocalStorageValueProvider can still keep the console.log as we don't know if the emit is appropriate there, and add the emit to rich history where we know we want it. We can do it in separate PR.

aocenas · 2020-03-13T10:41:26Z

public/app/features/explore/RichHistory/RichHistoryCard.tsx

+              }}
+              onClick={e => {
+                e.stopPropagation();
+              }}


This seems weird is it a bug in the form elements?

aocenas · 2020-03-13T10:42:48Z

public/app/features/explore/RichHistory/RichHistoryCard.tsx

+
+  useEffect(() => {
+    setComment(query.comment);
+  }, [query.comment]);


This feels weird. If we are changing the comment in this component who would change the comment outside of the component?

This feels weird. If we are changing the comment in this component who would change the comment outside of the component?

yep, weird. I have super over-complicated it. I just looked at it in a fresh perspective and it is fixed.

This seems weird is it a bug in the form elements?

No, it is expected behaviour. You cave a clickable card - when you click on the card, it adds query to query editor (we might change this behaviour and instead of clickable card we add the button). And on that card we add buttons and textarea. So if you click on them, it bubbles up and also runs the function to add query to query editor. So we need to add stop propagation.

I would still like to lift this one level up. So I would rather have this function throw the error, handle it in the both the RichHistory helpers and in LocalStorageValueProvider where this is used. That way LocalStorageValueProvider can still keep the console.log as we don't know if the emit is appropriate there, and add the emit to rich history where we know we want it. We can do it in separate PR.

I have removed it from this PR, I will open then a new PR with just error handling of store.

aocenas

looks good

…-fixes

* master: (113 commits) AlertNotifications: Translate notifications IDs to UIDs in Rule builder (#19882) CircleCI: Don't build Storybook on PR (#22865) Graphite: Rollup Indicator (#22738) Plugins: Return jsondetails as an json object instead of raw json on datasource healthchecks. (#22859) Backend plugins: Exclude plugin metrics in Grafana's metrics endpoint (#22857) Graphite: Fixed issue with query editor and next select metric now showing after selecting metric node (#22856) Webpack: Fix webpack for enterprise (#22863) Metrics: Storybook documented components (#22854) Search: Improve tags layout , #22804 (#22830) Stackdriver: Fix GCE auth bug when creating new data source (#22836) @grafana/runtime: Add cancellation of queries to DataSourceWithBackend (#22818) Rich history: Test coverage (#22852) Chore: Support Volta in package.json (#22849) CircleCI: Skip enterprise builds for forked PRs (#22851) Toolkit: docker image for plugin CI process (#22790) Revert "Explore: Add test coverage for Rich history (#22722)" (#22850) Datasource config was not mapped for datasource healthcheck (#22848) upgrades plugin sdk to 0.30.0 (#22846) Explore: Add test coverage for Rich history (#22722) Rich History: UX adjustments and fixes (#22729) ...

* Initial commit * Fix spelling of data sources * Display sorting value for starred and query tab * Fix handle color for light theme * Add close button and fix animation * Remove toggling of tabs * Stop event propagation when clicking on comment buttons * Add title for card functionality * Remove interpolation for easier searchability of variables * Improve syncing of comments and starred * Add modal to check if user wants to permanently delete history * Fix the height of the query card buttons * Adjust slider's width based on drawer width * Add spacing between slider and legend * Semantic variable naming * Fix disabled button when live tailing * Add error handling * Remove unused imports * Fix starring, remove useEffect * Remove emiting of appEvents.alertError in store * Remove unused imports (cherry picked from commit 5446900)

ivanahuckova added 12 commits March 11, 2020 16:37

Initial commit

5ffc89a

Fix spelling of data sources

53d11bb

Display sorting value for starred and query tab

c509707

Fix handle color for light theme

d13d85e

Add close button and fix animation

197a6ea

Remove toggling of tabs

94e74c3

Stop event propagation when clicking on comment buttons

6c468d5

Add title for card functionality

fe43f5f

Remove interpolation for easier searchability of variables

1b8a1bc

Improve syncing of comments and starred

4d09b9d

Add modal to check if user wants to permanently delete history

d6ff778

Fix the height of the query card buttons

b7aed38

ivanahuckova changed the title ~~Rich History: Fixes~~ Rich History: UX adjustments and fixes Mar 11, 2020

ivanahuckova added 6 commits March 12, 2020 12:34

Adjust slider's width based on drawer width

da31585

Add spacing between slider and legend

df390e7

Semantic variable naming

1c4a352

Fix disabled button when live tailing

9f544d2

Add error handling

1851554

Remove unused imports

ed73f5c

ivanahuckova commented Mar 13, 2020

View reviewed changes

ivanahuckova requested review from aocenas, jessover9000, torkelo and Estrax March 13, 2020 10:33

aocenas reviewed Mar 13, 2020

View reviewed changes

ivanahuckova added 3 commits March 13, 2020 12:50

Fix starring, remove useEffect

a15ab2f

Remove emiting of appEvents.alertError in store

fe55ca1

Remove unused imports

598f20d

ivanahuckova requested a review from aocenas March 13, 2020 12:06

jessover9000 mentioned this pull request Mar 13, 2020

Rich history UX fixes #22783

Merged

ivanahuckova mentioned this pull request Mar 16, 2020

Explore: Rich history #22570

Merged

aocenas approved these changes Mar 17, 2020

View reviewed changes

Merge remote-tracking :branch 'origin/master' into ivana/rich-history…

21580dd

…-fixes

ivanahuckova merged commit 5446900 into master Mar 17, 2020

ivanahuckova deleted the ivana/rich-history-fixes branch March 17, 2020 15:06

ivanahuckova self-assigned this Mar 17, 2020

ivanahuckova added add to changelog area/explore cherry-pick needed labels Mar 17, 2020

ivanahuckova added this to the 6.7 milestone Mar 17, 2020

ivanahuckova mentioned this pull request Mar 17, 2020

Rich history: Test coverage #22852

Merged

marefr mentioned this pull request Mar 19, 2020

6.7.0 cherrypicks #22893

Merged

aknuds1 removed the cherry-pick needed label Mar 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rich History: UX adjustments and fixes #22729

Rich History: UX adjustments and fixes #22729

ivanahuckova commented Mar 11, 2020 •

edited

Loading

ivanahuckova Mar 13, 2020

aocenas Mar 13, 2020

ivanahuckova Mar 13, 2020

ivanahuckova Mar 13, 2020

ivanahuckova Mar 13, 2020 •

edited

Loading

ivanahuckova Mar 13, 2020 •

edited

Loading

ivanahuckova Mar 13, 2020 •

edited

Loading

ivanahuckova Mar 13, 2020

ivanahuckova Mar 13, 2020 •

edited

Loading

ivanahuckova Mar 13, 2020

aocenas Mar 13, 2020

ivanahuckova Mar 13, 2020

ivanahuckova Mar 13, 2020

ivanahuckova Mar 13, 2020

ivanahuckova Mar 13, 2020

aocenas Mar 13, 2020

aocenas Mar 13, 2020

aocenas Mar 13, 2020

ivanahuckova Mar 13, 2020

ivanahuckova Mar 13, 2020

ivanahuckova Mar 13, 2020

aocenas left a comment

Rich History: UX adjustments and fixes #22729

Rich History: UX adjustments and fixes #22729

Conversation

ivanahuckova commented Mar 11, 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

ivanahuckova Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

ivanahuckova Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

ivanahuckova Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanahuckova Mar 13, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aocenas left a comment

Choose a reason for hiding this comment

ivanahuckova commented Mar 11, 2020 •

edited

Loading

ivanahuckova Mar 13, 2020 •

edited

Loading

ivanahuckova Mar 13, 2020 •

edited

Loading

ivanahuckova Mar 13, 2020 •

edited

Loading

ivanahuckova Mar 13, 2020 •

edited

Loading