Add Edit Container... command
#769
Conversation
MicroFish91
changed the title
Update Container... commandUpdate Container... command
There was a problem hiding this comment.
After seeing this quick pick in the UI, I feel like it may be kind of confusing.
Also, it's a bit odd to me that we have a yes, no, and no, don't ask again. There is probably a scenario where someone would want yes, and don't ask again as well.
I'll try messing with the quick pick a bit to see if we can add a checkbox or button that could be the "don't ask again". Not blocking it over that, just something that I wanted to bring up.
|
|
||
| let containersIdx: number; | ||
| if (ContainersItem.isContainersItem(item)) { | ||
| // The 'updateContainer' command should only show up on a 'ContainersItem' when it only has one container, else the command would show up on the 'ContainerItem' |
There was a problem hiding this comment.
This is a nit, but if it's an easy change you might want to consider it.
I think that it might be better to always show the command on the ContainersItem. Can we do that and when there are multiple containers just ask the user to pick one? You already have the pick container logic above, you might be able to reuse that.
I believe that users don't like when commands are missing, we want to be consistent so they can do things in the same way each time
There was a problem hiding this comment.
Yeah that's a good point, that's also how we have it implemented for picking revisions so this would help keep that pattern more consistent. I will make a change in a follow-up PR to add this
There was a problem hiding this comment.
nit: the command is named "update container" while the context is the reverse, "container update context". If there's not a good reason for this, then I'd prefer if they matched.
There was a problem hiding this comment.
I think I'm bouncing this one to @nturinski 🤭
I think he asked in a previous meeting people's preferences and was trying to improve naming conventions... but I can't remember who all was there to chime in
There was a problem hiding this comment.
Ah, yeah. I believe the conversation was basically anything that is a class/context should be noun (in this case, Container) prefixed so that in an alphabetical list, it's easy to see all of the Container related files grouped together. However, since commands have been following the "verbNoun" format, I was fine with the reverse order specifically for commands.
package.json
Outdated
| { | ||
| "command": "containerApps.updateContainer", | ||
| "title": "%containerApps.updateContainer%", | ||
| "category": "Azure Container Apps" | ||
| }, |
There was a problem hiding this comment.
Don't want to block this PR from merging, but I think we need to discuss the possibility of rewording the command to make it more clear to users what's happening.
We need users to understand that there are pretty much two main actions for container apps:
- Editing their Container App configuration (scale rules, etc.) - currently this is "Edit Container App..."
- Updating the image running in their containers (within the Container App) - currently this is "Update Container..."
Nathan and I think that "Update Container..." might be too close to "Edit Container App...". What about using Deploy instead of Update?
Whatever we decide to do, we can make that change after this is merged.
There was a problem hiding this comment.
I agree, I'll try to squeeze this discussion into one of our next group meetings (probably just the engineers for now). I think these sorts of improvements overlap a ton with my big goal for the next release, and I have some extra context to provide that would be helpful.
Although a user could want yes and don't ask again, I would think we should naturally prevent this scenario as I think it's a bad pattern to enable since it kind of goes against the point of batching revision edits. I do kind of agree that it looks a little funny though, but I'm not sure what the better alternative is yet, may be worth discussing in our next meeting with Anthony |
MicroFish91
changed the title
Update Container... commandEdit Container... command
Replace the existing
Update Container Image...on the container app item with anUpdate Container...command now that the containers view has been added.Before:
After:
Also adds support for the new activity log view. Examples:
(Updating using existing image)
(Updating using workspace project and building image from scratch)
Update: Changed to use the
Edit Container...instead ofUpdate Container...