Skip to content

Update deleteManagedEnvironment activity messages and general housekeeping#431

Merged
MicroFish91 merged 5 commits intomainfrom
mwf/managed-env
Aug 10, 2023
Merged

Update deleteManagedEnvironment activity messages and general housekeeping#431
MicroFish91 merged 5 commits intomainfrom
mwf/managed-env

Conversation

@MicroFish91
Copy link
Copy Markdown
Contributor

@MicroFish91 MicroFish91 commented Aug 9, 2023

Closes #389

From:
image

To:
image

@MicroFish91 MicroFish91 requested a review from a team as a code owner August 9, 2023 18:13
@MicroFish91 MicroFish91 marked this pull request as draft August 9, 2023 18:14
@MicroFish91 MicroFish91 changed the title Update deleteManagedEnvironment activity and housekeeping Update deleteManagedEnvironment activity messages and general housekeeping Aug 9, 2023
@MicroFish91 MicroFish91 marked this pull request as ready for review August 9, 2023 18:25
public async execute(context: IDeleteManagedEnvironmentContext, progress: Progress<{ message?: string | undefined; increment?: number | undefined }>): Promise<void> {
const client: ContainerAppsAPIClient = await createContainerAppsAPIClient([context, context.subscription]);

const deleting: string = localize('deletingManagedEnv', 'Deleting environment... this may take several minutes...');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I prefer having the name in here since it takes so long. Also don't like the added ellipsis at the end.

Copy link
Copy Markdown
Contributor Author

@MicroFish91 MicroFish91 Aug 9, 2023

Choose a reason for hiding this comment

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

Wait, do we really want to display the name again (it's already showing in the title right next to it)? I also tried without ellipses, and it looks kind of weird to me. What phrase are you looking at as the replacement description?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ahhh sorry, I forgot that's how it looked on the activity log.

Maybe we just chnage it to This may take a few minutes...?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm also cool with that, but let me give you some additional context just in case.

The reason I kept the Deleting environment... portion prefixed was because the description could first show the Deleting container app {x}... message if the managed environment has children - so keeping it does let the user know what the current step is actually doing and lets the user know that the process has progressed. That said, it is a bit long with it, so I'm happy to remove it... I personally don't have very strong opinions between these two descriptions

@MicroFish91 MicroFish91 merged commit 5ecaed7 into main Aug 10, 2023
@MicroFish91 MicroFish91 deleted the mwf/managed-env branch August 10, 2023 20:03
@microsoft microsoft locked and limited conversation to collaborators Mar 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make managed environment activity delete description shorter

2 participants