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

Support two variants of LanguageStatusItem#text #192880

Open
jrieken opened this issue Sep 12, 2023 · 14 comments
Open

Support two variants of LanguageStatusItem#text #192880

jrieken opened this issue Sep 12, 2023 · 14 comments
Assignees
Labels
api-proposal feature-request Request for new features or functionality languages-basic Basic language support issues ux User experience issues

Comments

@jrieken
Copy link
Member

jrieken commented Sep 12, 2023

The LanguageStatusItem#text properties is the text that shows when an item is pinned and when an item is rendered in the hover (as [ ]). There are cases in which it is hard to find a short text that works well for the pinned and unpinned case. We should explore to extend the API to text: string | { value:string, shortValue: string} so that both cases can be caters for

@jrieken
Copy link
Member Author

jrieken commented Sep 12, 2023

fyi @DanTup

@jrieken jrieken self-assigned this Sep 12, 2023
@jrieken jrieken added feature-request Request for new features or functionality languages-basic Basic language support issues ux User experience issues api-proposal labels Sep 12, 2023
@jrieken jrieken added this to the September 2023 milestone Sep 12, 2023
@DanTup
Copy link
Contributor

DanTup commented Sep 12, 2023

I'm not sure if this will cover the case I had, but it might depend exactly what you're planning to show in the tooltip (for ex. will it include detail?).

Assuming with the new API we now have variables for text.value, text.shortValue, detail, would the display be:

  • Pinned status bar: text.shortValue
  • Pinned status hover: ${text.value} - ${detail}
  • Unpinned entry: ${text.value} - ${detail}

If so, I think I could then use:

{
  text: { shortValue: "Android Device", text: "Flutter Device" }
  detail: "Android Device"
}

Which would give me what I want (the status bar text to show only the device name, and the tooltip/unpinned text to have both the label "Flutter Device" and the device name). It feels slightly odd that I'm essentially putting detail into text because I want the status item to show the useful part of the entry (not the label).

@jrieken
Copy link
Member Author

jrieken commented Sep 12, 2023

If so, I think I could then use:

That would be as bogus as it is today. The text property is the important part and detail is optional. You should be using text: 'Android Device' or text: { value: 'Android Device', shortValue: 'Android'} (or use a device theme icon for shortValue)

@DanTup
Copy link
Contributor

DanTup commented Sep 12, 2023

Putting "Android Device" in the text means the unpinned text won't have a label explaining what this is. Based on the current unpinned display, it seems like text needs to include a description of what the item is, not just its value.

I'm not currently using language status for this today because it doesn't do what I want/users expect. I guessed the reason for the ping above was because of #165940, but it doesn't seem like this change will address the scenario described there.

It's very possible I'm using this wrong, but the way we're currently using this feels good except for this one case (the device selection). Out other entries use text=description of item, detail=value and include SDK versions and the status of the language server:

image

(this screenshot is from that other issue where I'd tried to migrate the devices.. those are not currently in the shipped extension because it results in bad text when pinned)

Edit: I only just saw #165940 (comment), and this change feels less odd now I know I was using text/detail the wrong way around.

@jrieken
Copy link
Member Author

jrieken commented Sep 14, 2023

@DanTup Is is that you are preferring the "Flutter Device - macOS (darwin)" layout/rendering over "macOS (darwin) - Flutter Device". Do I understand correctly that this makes you use text/details properties swapped wrt to the API semantics?

@jrieken
Copy link
Member Author

jrieken commented Sep 14, 2023

I guessed the reason for the ping above was because of #165940, but

Totally, that issues became fuzzy due to various other asks and I wanted to do a focused effort.

@DanTup
Copy link
Contributor

DanTup commented Sep 14, 2023

is that you are preferring the "Flutter Device - macOS (darwin)" layout/rendering over "macOS (darwin) - Flutter Device".

Yep, although that goes for all the items in the list not just this. I had assumed the text on the left was intended as a description of the value on the right, like:

Dart SDK - v1.2.3
Flutter SDK - v2.3.4
Flutter Device - Android Foo
Dart Analysis Server - Starting

However if other languages (and the intention) are the other way around, then I should probably update to be consistent:

v1.2.3 - Dart SDK
v2.3.4 - Flutter SDK [change]
Android Foo - Flutter Device [change]
Dart Analysis Server [restart]

This doesn't look as good to me, but I think I should be using the labels in the same way as others so in future if the layout changes, it doesn't look odd.

@jrieken
Copy link
Member Author

jrieken commented Sep 14, 2023

However if other languages (and the intention) are the other way around, then I should probably update to be consistent:

Yeah, I know that TS uses it that way.

Screenshot 2023-09-14 at 16 30 47

I am happy to discuss and tweak the looks but semantically correct API usage is P1 because only that will us to improve the UI without regressing the experience for others

@DanTup
Copy link
Contributor

DanTup commented Sep 14, 2023

Yep, understood. I filed Dart-Code/Dart-Code#4741 about swapping them around.

I always thought they might look better more like a table (swapped around to the order the Dart ones are currently shown), but I don't know if that would work for all other uses of this.

Dart Analysis Server    terminated        [restart]
Dart SDK                v1.2.3            [change]
Flutter SDK             v2.3.4            [change]
Flutter Device          Android Pixel 4a  [change]

@jrieken jrieken modified the milestones: September 2023, October 2023 Sep 25, 2023
@jrieken
Copy link
Member Author

jrieken commented Oct 2, 2023

@bobbrow Would this proposal help you wrt what you said in #165940 (comment)

@bobbrow
Copy link
Member

bobbrow commented Oct 2, 2023

@bobbrow Would this proposal help you wrt what you said in #165940 (comment)

If the "shortValue" is the pinned text property we were asking for, then I believe the answer is "yes". Let us know when you have something we can try out.

@jrieken
Copy link
Member Author

jrieken commented Oct 9, 2023

@bobbrow The proposal (languageStatusText) will be in next (tomorrows) insiders

@jrieken jrieken modified the milestones: October 2023, November 2023 Oct 9, 2023
jrieken added a commit that referenced this issue Oct 9, 2023
…xt` (#195141)

* add proposed API for short and long variant of `LanguageStatusItem#text`

#192880

* add new proposal to allow list
Alex0007 pushed a commit to Alex0007/vscode that referenced this issue Oct 26, 2023
…xt` (microsoft#195141)

* add proposed API for short and long variant of `LanguageStatusItem#text`

microsoft#192880

* add new proposal to allow list
@jrieken jrieken removed this from the November 2023 milestone Nov 8, 2023
@jrieken
Copy link
Member Author

jrieken commented Nov 8, 2023

@bobbrow did you have a chance to try this?

@bobbrow
Copy link
Member

bobbrow commented Nov 8, 2023

Yes, we did try this. This does what we need.

The only additional thing we noticed with this was that we probably won't use the busy property on the language status any more because we didn't want the animating icon to show up in the status bar next to the icon we were using in that pinned state. It would be nice if we could have the busy state only apply to the flyout text for our case. Then it would be perfect. 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-proposal feature-request Request for new features or functionality languages-basic Basic language support issues ux User experience issues
Projects
None yet
Development

No branches or pull requests

3 participants