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

in the transcript context list, omit needless '@' prefix #3439

Closed
wants to merge 1 commit into from

Conversation

sqs
Copy link
Member

@sqs sqs commented Mar 17, 2024

When you click to open the Context: 123 lines from 456 files list, now the items no longer are prefixed with @. For example, it will show my/file.ts:12-34 rather than @my/file.ts:12-34. The @ is not needed when displaying the list here.

image

Test plan

CI

@sqs sqs requested review from a team and toolmantim March 17, 2024 05:24
@sqs
Copy link
Member Author

sqs commented Mar 17, 2024

@toolmantim can you design-check this?

@sqs sqs force-pushed the sqs/no-at-prefix-context-list branch from 1e31830 to b69baec Compare March 17, 2024 06:00
@sqs sqs force-pushed the sqs/no-at-prefix-context-list branch from b69baec to 7ac8e2c Compare March 17, 2024 06:05
Base automatically changed from sqs/no-wrap-https-urls to main March 17, 2024 06:17
@sqs sqs force-pushed the sqs/no-at-prefix-context-list branch 2 times, most recently from a0f2ddd to 004ecb5 Compare March 17, 2024 07:01
@toolmantim
Copy link
Contributor

I don’t have a Figma for it, but I think better than removing would be to use different icons to show where/why the file/lines were included. The more we can show where/why the better.

Codicon has a proper @ icon we could use for explicitly @‘d things. File lines included from a selection could use another icon. And things included from enhanced context could use the same DB icon as sources from the enhanced context popover.

With tooltips for each one, e.g. “Included from @-mentions” or “Included from the search index via Enhanced Context”.

wdyt? I’m happy for you to rough it out and I jump in to CSS it, or can Figma it if it’s not clear.

@sqs
Copy link
Member Author

sqs commented Mar 18, 2024

@toolmantim That sounds great. I'm not sure that icons for /why/ make the most sense, though. We will soon have multiple types of context (already do: files, symbols, terminal) and the icon should probably be what the thing is, not why it was included. (How) should we represent both?

@sqs sqs force-pushed the sqs/no-at-prefix-context-list branch from ddc78c0 to 74eebdc Compare March 19, 2024 03:05
When you click to open the `Context: 123 lines from 456 files` list, now the items no longer are prefixed with `@`. For example, it will show `my/file.ts:12-34` rather than `@my/file.ts:12-34`. The `@` is not needed when displaying the list here.
@sqs sqs force-pushed the sqs/no-at-prefix-context-list branch from 74eebdc to cdba0f6 Compare March 19, 2024 16:18
Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

Looks very clean!
image

@sqs sqs closed this Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants