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

Disable Uninstall and Enable/Disable buttons while uninstalling #1539

Merged
merged 1 commit into from
Nov 27, 2020

Conversation

panuhorsmalahti
Copy link
Contributor

  • Disable Uninstall and Enable/Disable buttons while uninstalling
  • Add "spinner" (e.g. waiting animation) to Uninstall button while install is in progress
  • Add Notification for uninstall

@panuhorsmalahti panuhorsmalahti requested a review from a team November 26, 2020 13:17
@jakolehm jakolehm added this to the 4.0.0 milestone Nov 26, 2020
@jakolehm jakolehm added area/extension Something to related to the extension api enhancement New feature or request labels Nov 26, 2020
Copy link
Collaborator

@Nokel81 Nokel81 left a comment

Choose a reason for hiding this comment

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

Except for a few minor things. This looks good.

Comment on lines 51 to 55
function getRemovedUninstalling(extensions: InstalledExtension[], extensionState: ObservableMap<string, ExtensionState>) {
return Array.from(extensionState.entries()).filter(([id, extension]) =>
extension.state === "uninstalling" && !extensions.find(extension => extension.id === id)
).map(([id, extension]) => ({ ...extension, id }));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this would be better as a @computed get removedUninstalling() { ... } on the Extensions class bellow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converted

description.toLowerCase().includes(searchText),
].some(v => v);
description?.toLowerCase().includes(searchText),
].some(value => value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would using _.filter be more readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but not going to touch unrelated code much here, just wanted to fix one char variable naming..

…Notification for uninstall.

Signed-off-by: Panu Horsmalahti <phorsmalahti@mirantis.com>
@panuhorsmalahti panuhorsmalahti merged commit 263d56b into master Nov 27, 2020
@panuhorsmalahti panuhorsmalahti deleted the feature/uninstall-button-disable branch November 27, 2020 08:23
@jakolehm jakolehm mentioned this pull request Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/extension Something to related to the extension api enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants