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

Feat/undo redo action #15704

Merged
merged 22 commits into from
Jul 14, 2023
Merged

Feat/undo redo action #15704

merged 22 commits into from
Jul 14, 2023

Conversation

maxiadlovskii
Copy link
Contributor

@maxiadlovskii maxiadlovskii commented Jun 6, 2023

Description

This PR adds 2 buttons to the sidebar with undo/redo actions. These actions change the view state in the proper direction.
The buffer size is 10. It can be changed if it wouldn't be enough.

Motivation and Context

#15713

How Has This Been Tested?

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.

@maxiadlovskii maxiadlovskii marked this pull request as ready for review June 19, 2023 15:24
Copy link
Contributor

@linuspahl linuspahl left a comment

Choose a reason for hiding this comment

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

I found some problems, but in general it works well!

When I add an aggregation, I need to click undo twice to undo the widget creation (but redo only once afterwards).

undo_twice

When I create a widget, focus it (or open edit mode) and click on undo (which reverts the widget creation), I see an error: Cannot read properties of undefined (reading 'toArray’).
undo creation focus

When I create a dashboard tab and click undo, I get the error Cannot read properties of undefined (reading 'widgets').

title={title}
sidebarIsPinned={sidebarIsPinned} />
);
})}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to visually distinguish the these actions from the actual sidebar sections.

We could add a separator.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Beside the sections array, we could also have an actions array.

Copy link
Contributor

Choose a reason for hiding this comment

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

We talked about removing the arrow in the sidebar, because you can open and close the sidebar by clicking in the section icon, but we can implement this in a different PR.

position: relative;
z-index: 4; /* to render over SidebarNav::before */
width: 100%;
height: 40px;
text-align: center;
cursor: pointer;
cursor: ${disabled ? 'not-allowed' : 'pointer'};
opacity: ${disabled ? 0.65 : 1};
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can apply the opacity only for the icon and not for the label?
image

Copy link
Member

@dennisoelkers dennisoelkers left a comment

Choose a reason for hiding this comment

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

Works fine, thanks! I added some suggestions for improvement of the code structure.

Copy link
Contributor

@linuspahl linuspahl left a comment

Choose a reason for hiding this comment

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

In some cases I still need two clicks to undo / redo a change, but in general the functionality seems to work well and all other comments have been addressed.

@dennisoelkers dennisoelkers merged commit 32e24eb into master Jul 14, 2023
3 checks passed
@dennisoelkers dennisoelkers deleted the feat/undo-redo-action branch July 14, 2023 13:51
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