Move widget to tab #8236

kmerz · 2020-05-29T10:09:37Z

Description

Add new menu item to widget menu: Move to Page. When clicking the user will see a modal
where he can select the target page. Optionally he can decide to keep a copy of the widget
on the current page. After selecting the target page he clicks on Select. The widget will
be moved to the new page and the active page will be set to the target query.

Motivation and Context

A user was able to copy a widget from a search to the first page of a dashboard. But the widget
was stuck now there. There was no possibility to move the widget to a different page of the dashboard.

How Has This Been Tested?

Create a dashboard
create a widget on the first page
move the widget to a different page

Fixes #8152

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Refactoring (non-breaking change)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.

linuspahl

I added a few smaller suggestions for improvements, the only bigger topic I would like to discuss is the handling of the widget positions.

linuspahl · 2020-06-02T11:24:43Z

graylog2-web-interface/src/views/components/widgets/Widget.jsx

@@ -328,6 +361,12 @@ class Widget extends React.Component<Props, State> {
                                     onCancel={this._onToggleCopyToDashboard} />
                  )}
                  {showCsvExport && <CSVExportModal view={view.view} directExportWidgetId={widget.id} closeModal={this._onToggleCSVExport} />}
+                  {showMoveWidgetToTab && (
+                    <MoveWidgetToTabForm view={view.view}


Let's call the component MoveWidgetToTabModal to unify the naming with comparable components.

linuspahl · 2020-06-02T11:29:24Z

graylog2-web-interface/src/views/components/widgets/MoveWidgetToTab.jsx

+    : <span>No dashboards found</span>;
+
+  return (
+    <Modal show>


Let's use the BootstrapModalForm here.

linuspahl · 2020-06-02T12:29:46Z

graylog2-web-interface/src/views/components/widgets/MoveWidgetToTab.jsx

+  return queryIds.map((queryId, index) => {
+    const tabTitle = view.state.get(queryId).titles.get('tab', Map({ title: `Page#${index + 1}` }));
+    return ({ id: queryId, name: tabTitle.get('title') });
+  });


We should either sort the list alphabetically or use the order defined by the dashboard.

What do you think about not including the current page in the list?
If "Keep Copy" is not active, nothing happens and otherwise the widget will be copied.
If the user wants to copy a widget on the same dashboard page he can also use the copy action.

We should either sort the list alphabetically or use the order defined by the dashboard

As you see I need to "make up" the names of the page titles on the fly. That means it is in order defined by the dashboard.

linuspahl · 2020-06-02T12:40:31Z

graylog2-web-interface/src/views/components/widgets/MoveWidgetToTab.jsx

+const _tabList = (view: View): Array<TabEntry> => {
+  const queryIds = Object.keys(view.state.toObject());
+  return queryIds.map((queryId, index) => {
+    const tabTitle = view.state.get(queryId).titles.get('tab', Map({ title: `Page#${index + 1}` }));


I thought about extracting the Page# part and using the reference here, in QueryTitle.js and QueryTabs.jsx. But having a look at QueryTitle.js you can also use the related queryTitle function, like we do it in BigDisplayMode.

linuspahl · 2020-06-02T12:47:16Z

graylog2-web-interface/src/views/components/widgets/MoveWidgetToTab.jsx

+                   onClick={() => setSelectedTab(id)}
+                   active={id === selectedTab}
+                   key={id} />
+  ));


Just a side note, it would be great if we could unify the styling of this list, the list which is visible when copying a widget to a dashboard and the saved searches list. We are sometimes defining the title as a header and sometimes as a child, which is supposed to be the description. I will create a separate issue for this change.

I now used the children way since it looks much nicer than the header way. Thanks for pointing that out.

linuspahl · 2020-06-02T12:56:07Z

graylog2-web-interface/src/views/logic/views/FindWidgetAndQueryIdInView.js

+
+type QueryId = string;
+
+const FindWidgetAndQueryIdInView = (widgetId: string, view: View): ?[Widget, QueryId] => {


You could use the newly created WidgetId type from views/types here.

linuspahl · 2020-06-02T12:56:26Z

graylog2-web-interface/src/views/logic/views/MoveWidgetToTab.js

+    .build();
+};
+
+const MoveWidgetToTab = (widgetId: string, targetQueryId: QueryId, dashboard: View, copy: boolean = false): ?View => {


You could use the newly created WidgetId type from views/types here as well.

linuspahl · 2020-06-02T12:59:53Z

graylog2-web-interface/src/views/logic/views/MoveWidgetToTab.js

+
+const MoveWidgetToTab = (widgetId: string, targetQueryId: QueryId, dashboard: View, copy: boolean = false): ?View => {
+  if (dashboard.type !== View.Type.Dashboard) {
+    return undefined;


I would prefer to expect that this function returns a View and to throw an error if the search type does not match or if the the related Query does not exist.

linuspahl · 2020-06-02T13:28:04Z

graylog2-web-interface/src/views/logic/views/MoveWidgetToTab.js

+const _removeWidgetFromTab = (widgetId: string, queryId: string, dashboard: View): View => {
+  const viewState = dashboard.state.get(queryId);
+  const widgetIndex = viewState.widgets.findIndex((widget) => widget.id === widgetId);
+  const widgetPosition = viewState.widgetPositions;


Let's call this variable widgetPositions

linuspahl · 2020-06-02T14:40:36Z

graylog2-web-interface/src/views/logic/views/MoveWidgetToTab.js

+  return undefined;
+};
+
+export default MoveWidgetToTab;


One bigger topic I would like to discuss is the handling of the widget positions when removing / adding a widget.

When the widget is being moved to a page, we do not define / calculate a widget position. This can lead to strange behaviours. During my tests the newly created widget is mostly being displayed in the second row of the widget grid. But when I open and close the widget edit modal, it moves to the first row. It would be great if we could reuse some parts of the logic we are using when creating a widget. (CurrentViewStateStore -> widgets())

When a user removes a widget, by not using the "Keep Copy" option, we are just removing the widget position, which is ok, because it behaves like when using the widget action Delete. But in my opinion we should check if this effects other widgets and recalculate their positions. E.g. when the widget was the only one in its row.
The WidgetGrid is currently able to handle this "empty space", but it is possible that this will lead to problems in the future. For now I would not change this part, but I will create an issue to adjust this behaviour which includes writing a migration which checks if empty spaces theoretically exists on a dashboard and which recalculates the related widget positions.

What do you think?

Prior to this change, there was no code providing the possibility to move a widget from one tab to another. This change will add a function.

Prior to this change, there was no Modal where a user could select to which tab he would like to move a widget. This change will create a new Modal and connects it to the former MoveWidgetToTab logic.

Prior to these changes, we did not update the search id, which needed to be change, since we changed the search. This change will mock the id generation so we do not have failing snapshots tests with every run.

Prior to this change, you could only move a widget to a different page. Now you can also copy it instead. This change will add a checkbox where the user can decide to copy the widget instead of moving it.

linuspahl

Thanks for the update! I've added a few follow-up comments.

linuspahl · 2020-06-08T07:44:54Z

graylog2-web-interface/src/views/logic/views/MoveWidgetToTab.js

+      .col(0)
+      .row(0)
+      .build();
+    const tempDashboard = copy ? dashboard : _removeWidgetFromTab(widgetId, queryId, dashboard);


What is the reason you've implemented .col(0).row(0) here?
It would be great if moving a widget to a dashboard tab would add the widget to the tab in the same way, like when adding a new widget to a tab. We could reuse some of the logic to recalculate the widget positions from CurrentViewStateStore -> widgets()

linuspahl · 2020-06-08T07:48:07Z

graylog2-web-interface/src/views/components/widgets/MoveWidgetToTabModal.jsx

+    <BootstrapModalForm show
+                        onCancel={onCancel}
+                        onSubmitForm={() => onSubmit(widgetId, selectedTab, keepCopy)}
+                        title="Choose Target Page">


Let's reimplement submitButtonDisabled={!selectedTab} here.

linuspahl · 2020-06-08T08:09:09Z

graylog2-web-interface/src/views/components/widgets/MoveWidgetToTabModal.jsx

+  const [keepCopy, setKeepCopy] = useState(false);
+  const { id: activeQuery } = useStore(CurrentQueryStore);
+
+  const list = _tabList(view).filter(({ id }) => id !== activeQuery);


Regarding the sort, it does not look correct to me, compared with the dashboard:

One solution is using the order / values provided by the QueryIdsStore.

With the last review I suggested to remove the current tab from the list, but after having another look I prefer to just disable the active tab. It has the benefits, that the user always sees the "same" list no matter on which tab he opens the modal and it is easier to find the surrounding tabs.

Regarding the sort: #8264 there is no order :( Which we need to fix before this PR I think now. :(

Regarding disable: I think you were right before, the user does not expect to find the current tab here. Therefore I would leave it out to reduce the visual noise. Let's discuss it later in or after the meeting.

Prior to this change, we simply add a new WidgetPosition with row 1 and col 1. This will only lead to the problem that now 2 widgets have this coordinates. This change will reuse the calculation method from CurrentViewStateStore.

linuspahl

Thanks again for the update! Two last points:

The widget title is getting lost when moving a widget
When I open the modal and click directly on the "keep copy" checkbox, the click has no effect

linuspahl

Looks good to me!

kmerz added ready-for-review search User Experience labels May 29, 2020

kmerz added this to the 4.0.0 milestone May 29, 2020

kmerz requested review from dennisoelkers and linuspahl May 29, 2020 10:09

linuspahl requested changes Jun 2, 2020

View reviewed changes

kmerz added 11 commits June 5, 2020 14:59

Add move widget to tab functionallity

8adf9e6

Prior to this change, there was no code providing the possibility to move a widget from one tab to another. This change will add a function.

Create MoveWidgetToTab Modal

a6d95db

Prior to this change, there was no Modal where a user could select to which tab he would like to move a widget. This change will create a new Modal and connects it to the former MoveWidgetToTab logic.

Fix failing snapshot tests

9a871e1

Prior to these changes, we did not update the search id, which needed to be change, since we changed the search. This change will mock the id generation so we do not have failing snapshots tests with every run.

Make it possible to keep a copy of a widget

3e6fbff

Prior to this change, you could only move a widget to a different page. Now you can also copy it instead. This change will add a checkbox where the user can decide to copy the widget instead of moving it.

Rename MoveWidgetToTab to MoveWidgetToTabModal

85f267c

Use BootstrapModalForm and beautify ListGroupItem usage

6595fb8

Filter active query from selection list

b17769a

Use QueryTitle for title generation

b410375

Reuse existing types instead of reinventing

88c794c

use better VariableName and throw Error when type is unexpected

adbc3c6

When moving a widget also add new WidgetPostion

754bd74

kmerz force-pushed the move-widget-to-tab branch from 63f1adc to 754bd74 Compare June 5, 2020 13:00

fix linter warnings

8a27bea

linuspahl requested changes Jun 8, 2020

View reviewed changes

kmerz added 5 commits June 9, 2020 15:48

Use QueryIds store to keep the order of pages

a5675dd

Disable submit button if nothing is selected

4ca1702

Use the first row notation from WidgetGrid (starting by 1)

d1c3c14

zero was correct

7c27fb7

Recalcutate WidgetPositions

d78309d

Prior to this change, we simply add a new WidgetPosition with row 1 and col 1. This will only lead to the problem that now 2 widgets have this coordinates. This change will reuse the calculation method from CurrentViewStateStore.

linuspahl requested changes Jun 10, 2020

View reviewed changes

kmerz added 2 commits June 12, 2020 10:47

Also move the title of the widget

6021e6e

useCallback to memorize the callbacks

420b22c

linuspahl approved these changes Jun 12, 2020

View reviewed changes

linuspahl merged commit 4c1d41a into master Jun 12, 2020

linuspahl deleted the move-widget-to-tab branch June 12, 2020 13:28

linuspahl mentioned this pull request Jun 15, 2020

Add dashboard function to move widget to other query tab #7399

Closed

This was referenced Sep 15, 2020

Can't add widgets to other than default dashboard tabs #8925

Closed

Allow moving widgets between pages #8895

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move widget to tab #8236

Move widget to tab #8236

kmerz commented May 29, 2020

linuspahl left a comment •

edited

Loading

linuspahl Jun 2, 2020

linuspahl Jun 2, 2020

linuspahl Jun 2, 2020

linuspahl Jun 2, 2020

kmerz Jun 5, 2020

linuspahl Jun 2, 2020

linuspahl Jun 2, 2020

kmerz Jun 5, 2020

linuspahl Jun 2, 2020

linuspahl Jun 2, 2020

linuspahl Jun 2, 2020

linuspahl Jun 2, 2020

linuspahl Jun 2, 2020

linuspahl left a comment

linuspahl Jun 8, 2020 •

edited

Loading

linuspahl Jun 8, 2020

linuspahl Jun 8, 2020

linuspahl Jun 8, 2020

kmerz Jun 9, 2020 •

edited

Loading

linuspahl left a comment

linuspahl left a comment


		type QueryId = string;

		const FindWidgetAndQueryIdInView = (widgetId: string, view: View): ?[Widget, QueryId] => {

Move widget to tab #8236

Move widget to tab #8236

Conversation

kmerz commented May 29, 2020

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

linuspahl left a comment • 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

Choose a reason for hiding this comment

linuspahl left a comment

Choose a reason for hiding this comment

linuspahl Jun 8, 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

kmerz Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

linuspahl left a comment

Choose a reason for hiding this comment

linuspahl left a comment

Choose a reason for hiding this comment

linuspahl left a comment •

edited

Loading

linuspahl Jun 8, 2020 •

edited

Loading

kmerz Jun 9, 2020 •

edited

Loading