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

Add copy to clipboard option for project/executable source columns #1332

Merged
merged 5 commits into from
Dec 15, 2023

Conversation

adamint
Copy link
Member

@adamint adamint commented Dec 11, 2023

It looks like this now:
image

I also added this for executables, which removed the gray arg subtext so that the entire command appeared the same, as GridValue accepted only strings (I did not understand why there was subtext to begin with).

closes #1309

Microsoft Reviewers: Open in CodeFlow

@tlmii
Copy link
Member

tlmii commented Dec 11, 2023

I did not understand why there was subtext to begin with

It was there to make it easier to visually differentiate the different pieces of information we were cramming into one column (executable vs args vs working directory). With the different types of data in a single column (both for one resource type, like here, and across different resource types) visual scanning was difficult.

It's not strictly necessary, of course, but if we're going to remove it we should consider what else we can do to make the information more easily scannable.

@adamint
Copy link
Member Author

adamint commented Dec 11, 2023

It was there to make it easier to visually differentiate the different pieces of information we were cramming into one column

Right, the executable path + command seemed to me like two parts of the same information (what is being executed). Since working directory is now on a second line, do you still have that concern? @tlmii

@tlmii
Copy link
Member

tlmii commented Dec 11, 2023

Since working directory is now on a second line?

Working directory was on a second line when we originally crammed everything together (#987) - command and args were on one line (but differentiated by color), working directory was set off on the second line and labeled.

Right, the executable path + command seemed to me like two parts of the same information (what is being executed).

I agree that, at one level, executable + args are two parts of one concept (because of course together they form the full command). But from a "what am I looking for in the column?" perspective, a common thing is to look for just the executable name, and visually parsing that off the full command is easier when it is set off somehow. It's the same concept as how commands and args get colored differently in powershell for example:

image

I'm not saying this visual scanning is the only important thing, nor that this is the only way to solve it. But it does follow a common pattern. Perhaps we can loop in @leslierichardson95 to get her take.

@adamint
Copy link
Member Author

adamint commented Dec 11, 2023

Ah, apologies for the misunderstanding. We are able to keep the current behavior, it just requires a small change to GridValue to allow for fragments. I can go ahead and make that

@tlmii
Copy link
Member

tlmii commented Dec 11, 2023

Ah, apologies for the misunderstanding.

I think it is a valuable conversation to have - there's more than one way to accomplish these goals and you never know what'll come out of the discussions!

@adamint
Copy link
Member Author

adamint commented Dec 11, 2023

Alright, added the option to have a fragment after the value. back to the old view @tlmii

image

@adamint
Copy link
Member Author

adamint commented Dec 13, 2023

@tlmii @JamesNK @drewnoakes for re-review

Copy link
Member

@tlmii tlmii left a comment

Choose a reason for hiding this comment

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

I'm not 100% sure about the ContentAfterValue concept. But given how this component works, with the FluentHighlighter for the main value, if you did a normal fragment for the whole of Value's display, then you'd need to handle turning off highlighting, too, which just seems like too much. So at least for now I think this is a good compromise. We can always revisit if necessary.

@JamesNK
Copy link
Member

JamesNK commented Dec 14, 2023

Could this also be applied to containers? The button could copy the image name and tag, e.g. postgres:latest.

Right now it feels weird that some rows in a grid have a copy button in that column, and others don't.

@tlmii
Copy link
Member

tlmii commented Dec 14, 2023

Could this also be applied to containers? The button could copy the image name and tag, e.g. postgres:latest.

Right now it feels weird that some rows in a grid have a copy button in that column, and others don't.

I think at some point we're going to have to pull these columns back apart somehow. Having multiple different pieces of info (both within a single resource type and across resource types) within a column leads us down these rabbit trails like this. Hopefully we get some good feedback on it.

Adam Ratzman false added 3 commits December 14, 2023 13:25
# Conflicts:
#	src/Aspire.Dashboard/Resources/ControlsStrings.resx
#	src/Aspire.Dashboard/Resources/xlf/ControlsStrings.cs.xlf
#	src/Aspire.Dashboard/Resources/xlf/ControlsStrings.de.xlf
#	src/Aspire.Dashboard/Resources/xlf/ControlsStrings.es.xlf
#	src/Aspire.Dashboard/Resources/xlf/ControlsStrings.fr.xlf
#	src/Aspire.Dashboard/Resources/xlf/ControlsStrings.it.xlf
#	src/Aspire.Dashboard/Resources/xlf/ControlsStrings.ja.xlf
#	src/Aspire.Dashboard/Resources/xlf/ControlsStrings.ko.xlf
#	src/Aspire.Dashboard/Resources/xlf/ControlsStrings.pl.xlf
#	src/Aspire.Dashboard/Resources/xlf/ControlsStrings.pt-BR.xlf
#	src/Aspire.Dashboard/Resources/xlf/ControlsStrings.ru.xlf
#	src/Aspire.Dashboard/Resources/xlf/ControlsStrings.tr.xlf
#	src/Aspire.Dashboard/Resources/xlf/ControlsStrings.zh-Hans.xlf
#	src/Aspire.Dashboard/Resources/xlf/ControlsStrings.zh-Hant.xlf
@adamint adamint enabled auto-merge (squash) December 14, 2023 20:18
@adamint adamint merged commit fafff27 into dotnet:main Dec 15, 2023
8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to copy Source file path in dashboard resources page
4 participants