Skip to content
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

Dashboard auto-saving #3653

Merged
merged 6 commits into from
Apr 17, 2019
Merged

Dashboard auto-saving #3653

merged 6 commits into from
Apr 17, 2019

Conversation

ranbena
Copy link
Contributor

@ranbena ranbena commented Mar 28, 2019

What type of PR is this?

  • Feature

Description

UX discussion https://discuss.redash.io/t/auto-saving-dashboard-layout/3506
Demo https://deploy-preview-3653--redash-preview.netlify.com/dashboard/artists

Motivation

Currently when in dashboard edit mode, there are “Apply Changes” and “Cancel” buttons which are inconsistent (only apply to resize and drag - not to added / deleted widgets) and buggy (there is a scenario causing widget overlap).

The Change

Ditch cancellability and embrace auto-save, ala-Google Docs.
One button to switch off edit mode, next to it a save status indicator.

How it works

Each layout change marks a dirty state, showing a "Saving..." indication (although technically not yet saving) and triggers a server save to each changed widgets, debounced by 2 seconds.

Any error saving yields the appropriate notification and dirty indication persists.

Updated tests

Screen Shot 2019-04-12 at 11 00 18

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

ezgif com-video-to-gif

@ranbena ranbena changed the title Dashboard auto-saving (wip) Dashboard auto-saving Apr 10, 2019
const changedWidgets = getWidgetsWithChangedPositions(this.dashboard.widgets);
saveDashboardLayout(changedWidgets);
}, 50);
$timeout(saveDashboardLayout, 50);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the diffing into the debounced save.

@ranbena ranbena marked this pull request as ready for review April 10, 2019 13:44
@ranbena ranbena requested a review from kravets-levko April 10, 2019 13:44
@ranbena
Copy link
Contributor Author

ranbena commented Apr 10, 2019

@kravets-levko since widget deletion is immediate and not debounced, this scenario:

  1. Move widget A
  2. Delete widget A after 1s

The widget.save() would fail (server error) but you gotta be really fast and it's an edge case, so I don't think it requires any attention.

@ranbena ranbena self-assigned this Apr 10, 2019
@kravets-levko
Copy link
Collaborator

@ranbena I think there should be a way to check if widget was removed, but probably it doesn't worth spending time on it. Maybe, just put a comment somewhere with notes about that case - we can re-visit it when migrating services/models from Angular.

@ranbena
Copy link
Contributor Author

ranbena commented Apr 14, 2019

Made the "Done Editing" button disabled while saving.
But since disabled style is too distracting, I instead just made it unclickable (pointer-events: none).

Not sure this is the best way to go about it. I'd rather make it disabled on hover but no simple way to do that.

Copy link
Member

@gabrieldutra gabrieldutra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments, but both are not impediments 🙂.

In the future it would be interesting to have an onbeforeunload here too, similar to the Query Page one, to alert the user in case they are leaving and the content is not saved yet.

ng-click="$ctrl.editLayout(false, true)" ng-if="$ctrl.layoutEditing">
<i class="zmdi zmdi-check"></i> Apply Changes
</button>
<div ng-if="$ctrl.layoutEditing" ng-switch="$ctrl.isLayoutDirty || $ctrl.saveInProgress">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an issue, but perhaps it would be interesting to have a "Failed to save" state as well. E.g.: When internet connection fails as the state would keep Dirty you would see a "Saving" state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point - in case of failure we would mislead the user.
I'll see if I have a quick and easy solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #3715

if (!this.dashboard.canEdit()) {
return;
}

// calc diff, bail if none
const changedWidgets = getWidgetsWithChangedPositions(this.dashboard.widgets);
if (!changedWidgets.length) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lodash isEmpty is safer, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I know for sure it's gonna be an array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants