posit-dev / publisher

MIT License
3 stars 0 forks source link

Add view-logs anchor to publishing progress indicator #1896

Closed sagerb closed 14 hours ago

sagerb commented 4 days ago

Intent

Resolves #1796

Add an anchor to show the Publishing Logs while deployment is in progress.

Type of Change

Approach

Add an anchor only shown with the progress indicator, and execute the existing command to show the publishing log.

2024-06-27 at 3 06 PM

Automated Tests

Directions for Reviewers

Deploy and click on anchor while deployment in progress.

Checklist

dotNomad commented 18 hours ago

I'm not sure about the appearance. Could it be a small button? Or, if it's a link, I feel like it should have more spacing from the text above it.

I think a button would be too large. The buttons we have are how VSCode styles buttons, unless you are talking about an icon button, but I think the text is more helpful here.

We could definitely add a bit of margin between the in progress text and the logs link though.

sagerb commented 17 hours ago

Code LGTM, and I verified locally.

I'm not sure about the appearance. Could it be a small button? Or, if it's a link, I feel like it should have more spacing from the text above it.

I like the View Log wording. Since we have the same command in the ellipsis menu, can we use the same text to describe it?

@mmarchetti

I went with the link because I also thought a button would be too big there. I'll add a bit of margin.

On your wording request, we have multiple logs available to be seen within the ellipsis menu. Are you asking for me to switch to the phrase "View" rather than "Go to"? For context, here is the ellipsis menu currently: 2024-07-01 at 12 05 PM

Changing to View would result in: 2024-07-01 at 12 10 PM

dotNomad commented 16 hours ago

On your wording request, we have multiple logs available to be seen within the ellipsis menu.

I read it as making the wording consistent so instead of "View Log" it would be "View Publishing Log" for the link and the context menu. I agree that consistency would be nice.

I'll let @mmarchetti confirm if I was reading that correctly.

mmarchetti commented 16 hours ago

I think a button would be too large

Yes, a typical button would be. I was thinking along the lines of the buttons in the Extensions panel:

CleanShot 2024-07-01 at 15 21 05

I am also fine with keeping it as a link. As to wording, I would like to be consistent in the names we use for the same action. Seems like the options we've used or considered for showing a view are:

Of the 3, I think my preference for VSCode-local views would be Show because it shows an existing view. The View Content and View Content Log actions open new browser tabs and could retain different wording to differentiate them. I'm totally open to other options though.

sagerb commented 16 hours ago

I'm not sure about the appearance. Could it be a small button? Or, if it's a link, I feel like it should have more spacing from the text above it.

I think a button would be too large. The buttons we have are how VSCode styles buttons, unless you are talking about an icon button, but I think the text is more helpful here.

We could definitely add a bit of margin between the in progress text and the logs link though.

@dotNomad Here's a 5px top margin added: 2024-07-01 at 12 41 PM

Commit: https://github.com/posit-dev/publisher/pull/1896/commits/b523064d69431213f8ef6f794aad914000131295

sagerb commented 16 hours ago

Of the 3, I think my preference for VSCode-local views would be Show because it shows an existing view. The View Content and View Content Log actions open new browser tabs and could retain different wording to differentiate them. I'm totally open to other options though.

OK, I've updated to the following:

See https://github.com/posit-dev/publisher/pull/1896/commits/d50fee625b33494d786170c1eb0e592f0610c834

2024-07-01 at 1 14 PM

sagerb commented 16 hours ago

Yes, a typical button would be. I was thinking along the lines of the buttons in the Extensions panel:

@mmarchetti , are you thinking you want to replace the ellipsis menu then, with that dropdown menu? If so, let's do that in a separate PR.

mmarchetti commented 15 hours ago

I was just showing a small button that could be used in place of the Show Log link. But, I'm fine with this as is. It looks better with the added margin.