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

Snapshots, dialogs and UI revisit #5685

Merged
merged 28 commits into from
Oct 16, 2023

Conversation

torchiaf
Copy link
Contributor

@torchiaf torchiaf commented Oct 9, 2023

Fixes #5677

Screenshots

Empty page
image

Cards
image

Cards + notes
image

Confirm delete
image

Confirm Restore
image

Creation dialog
image

Restoring dialog
image

Creating/Restoring dialog at startup
image

Error example
image

@torchiaf torchiaf changed the title Snapshots, dialogs Snapshots, dialogs and UI workflow Oct 9, 2023
@torchiaf torchiaf force-pushed the feature/snapshots-dialogs branch 11 times, most recently from 23f2a73 to d400172 Compare October 11, 2023 17:02
@gaktive gaktive removed the request for review from ericpromislow October 11, 2023 18:14
Copy link
Contributor

@rak-phillip rak-phillip left a comment

Choose a reason for hiding this comment

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

Waiting on rebase of #5671 before giving a more thorough review, but I think we can improve some of the messages used throughout the dialogs.

pkg/rancher-desktop/assets/translations/en-us.yaml Outdated Show resolved Hide resolved
pkg/rancher-desktop/assets/translations/en-us.yaml Outdated Show resolved Hide resolved
pkg/rancher-desktop/assets/translations/en-us.yaml Outdated Show resolved Hide resolved
pkg/rancher-desktop/assets/translations/en-us.yaml Outdated Show resolved Hide resolved
pkg/rancher-desktop/assets/translations/en-us.yaml Outdated Show resolved Hide resolved
pkg/rancher-desktop/assets/translations/en-us.yaml Outdated Show resolved Hide resolved
pkg/rancher-desktop/assets/translations/en-us.yaml Outdated Show resolved Hide resolved
pkg/rancher-desktop/assets/translations/en-us.yaml Outdated Show resolved Hide resolved
pkg/rancher-desktop/assets/translations/en-us.yaml Outdated Show resolved Hide resolved
@torchiaf torchiaf force-pushed the feature/snapshots-dialogs branch 4 times, most recently from 972538f to d8f5d5b Compare October 12, 2023 14:19
@torchiaf torchiaf changed the title Snapshots, dialogs and UI workflow Snapshots, dialogs and UI revisit Oct 12, 2023
@torchiaf
Copy link
Contributor Author

@rak-phillip I added the title bar to confirmation dialogs. Blocking dialogs ATM are still not-movable and not-closable.

Copy link
Contributor

@rak-phillip rak-phillip left a comment

Choose a reason for hiding this comment

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

Testing on Windows 11: Taking a snapshot fails (which is fine regarding the scope of this PR), but I'm unable to close the create dialog. I expect to be able to close the dialog on an error.

image

background.ts Outdated

if (options.format.type !== 'question') {
/** On Linux it does nothing */
mainWindow.setOpacity(0.7);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if changing the opacity generates the desired effect here. The entire main window becomes transparent, where I'm thinking that the intent is to dim the main window as a means to communicate that it is "disabled" while the modal is open

image

We might need to create some form of overlay using an electron BrowserView1, via css, or some other method

Footnotes

  1. https://www.electronjs.org/docs/latest/api/browser-view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you Phil.

  • I've fixed the close button on Windows; it was caused by closable: true option, which is not required in the snapshots case.
  • I've fixed the dialog position on windows, now it should be always in the middle of the mainWindow.

Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
- Implement dialog workflow
- Implement dialog structure

Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
torchiaf and others added 12 commits October 13, 2023 09:47
- Clear old banners on handling dialogs
- clean-up code

Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Co-authored-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Co-authored-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Co-authored-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Co-authored-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Co-authored-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
- Fix dialog position on Windows
- Fix close button on Windows
- Remove opacity on Windows
- Code refactoring

Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
@torchiaf
Copy link
Contributor Author

Tested on Linux/MacOS/Windows

Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Copy link
Contributor

@rak-phillip rak-phillip left a comment

Choose a reason for hiding this comment

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

Looks good overall. Just a few comments to address.

pkg/rancher-desktop/assets/translations/en-us.yaml Outdated Show resolved Hide resolved
<span
v-if="snapshot.formattedCreateDate"
class="value"
v-html="t('snapshots.card.created', { date: snapshot.formattedCreateDate.date, time: snapshot.formattedCreateDate.time }, true)"
Copy link
Contributor

Choose a reason for hiding this comment

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

We will need to make sure that we follow up with #4323 in our next release.

pkg/rancher-desktop/components/SnapshotCard.vue Outdated Show resolved Hide resolved
pkg/rancher-desktop/window/index.ts Outdated Show resolved Hide resolved
pkg/rancher-desktop/window/index.ts Outdated Show resolved Hide resolved
torchiaf and others added 2 commits October 16, 2023 09:20
Co-authored-by: Phillip Rak <rak.phillip@gmail.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
@torchiaf
Copy link
Contributor Author

Thanks @rak-phillip , I've pushed the requested changes +

  • Corrected bold strings
  • Replaced the info banner with a simple message into Restore confirmation dialog.

Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Signed-off-by: Francesco Torchia <francesco.torchia@suse.com>
Copy link
Contributor

@rak-phillip rak-phillip left a comment

Choose a reason for hiding this comment

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

LGTM

@rak-phillip rak-phillip merged commit 84302c7 into rancher-sandbox:main Oct 16, 2023
10 checks passed
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.

Snapshots UI, dialogs
2 participants