Append hint text to placeholder if input supports history

[gjsjohnmurray]

This PR addresses the undiscoverability of the UpArrow/DownArrow history feature provided by some fields, in particular Find and Search. It does this by appending hint text to the placeholder.

[gjsjohnmurray]

pinging @misolori for opinion about the UX

[miguelsolorio]

My initial impression is that this looks very busy. Have you explored doing something similar to the search include/exclude for displaying the text on focus? The other part is once you have learned the shortcut, this would appear all the time. I'd also look into trying out the rich hover as well as an alternate option.

Do you have an open issue for this?

[gjsjohnmurray]

@misolori I take your point about looking busy. I wondered about appending just the symbol, but am concerned that the hint is then too cryptic. Good idea about only adding it when focused. I've pushed a change to do that, and also check that history actually exists before adding the hint.

There isn't a specific issue at this point, but I've responded to numerous ones logged by others who have asked for history and didn't know it already exists.

[miguelsolorio]

I'll post this in the ux channel for the team, I think the arrow symbol also feels a bit cryptic as it's not normally used in other places.

[gjsjohnmurray]

[miguelsolorio]

Feedback has been positive so far, a couple of things:

• When you don't have any search history and then you search => clear results the input is not updated (you have to click away and then click again to have it appear). Would be nice if this instantly applied.
• For include/exclude, if you append this to the text it would look better than the double paranthesis
  
• This also appears to break the include/exclude text that appears only when in focus
  

[gjsjohnmurray]

@misolori I have pushed fixes for the points you raised. I took a different approach with the double-parens because I felt that moving the history tip inside the existing parens made the placeholder hard to understand syntactically:

In testing I also noticed that the Clear Search History command missed the Replace field, so I fixed this.

[miguelsolorio]

A couple of additional comments that came up:

• Should this also be added to other areas that have history (debug console, terminal, etc.)?
• This might be too busy for each of those area, so we'll need to discuss whether we need to scope it to just search input (search, find, problems, etc.)

[gjsjohnmurray]

The feature is implemented in HistoryInputBox so it already works in the filter fields on Problems, Debug Console and Keyboard Shortcuts. The first two of those still use parens in their placeholder text.

I rather like how the history hint appears wherever HistoryInputBox is used. It might not have occurred to me to try UpArrow on those other three fields.

If busyness is a concern we could try it with just the up/down arrow glyph on its own.

[miguelsolorio]

I'll bring this up next week in our UX Sync since we are already in endgame

[miguelsolorio]

Also, someone mentioned that the arrows could conflict if the user changes the keybinding. So making sure it picks up the right one or hide it if it's not the default?

[gjsjohnmurray]

Not sure how we decide which glyph we'd use if a user has customized their keybindings for history.showNext and/or history.showPrevious. Seems reasonable not to hint at all if they've done that, as it implies they know about this history feature. But I'd need advice about how HistoryInputBox can discover those custom keybindings?

[miguelsolorio]

Maybe @roblourens can help with figuring out if we can get the keybindings from the HistoryInputBox.

CleanShot.2021-08-06.at.08.08.08.mp4

Tested this again today and I did noticed that when you have focus on an input, search, then clear your input, that the placeholder doesn't get updated until you move focus away. Wonder if we're able to account for this state? It feels more like a polish item so no worries if it's too complicated.

Also would love @roblourens' feedback before we merge this. On one hand, this adds more discoverability to search history but on the other can be a little noisy. Does this warrant a setting? I think not but maybe others disagree.

[roblourens]

I'm not sure about it. I'm all for making history more discoverable but it just feels like a lot of text

Something that bugs me is that I feel like the placeholder text color in default themes is a bit dark which makes this text feel heavy (accessibility requirements I think)

If we take this, it feels like that unicode character isn't lined up quite right with the rest of the text. Maybe there's another one.

[miguelsolorio]

Here's feedback from the ux sync today:

• You could omit the "for" to make the placeholder text cleaner "(↕️ history"
• Would be nice to have this only showing this after XX times of usage (we don't currently do this)
• There's been feedback for viewing all history, this is a separate issue though

Seems like people generally like the idea of this but could be noisy after you've done this a few times. Not sure it would warrant a setting to toggle (feels pretty minor)

[gjsjohnmurray]

Thanks for the feedback. Trying a different character and a wordless presentation:

I like this, and as a bonus it doesn't need translating.

Would be nice to have this only showing this after XX times of usage (we don't currently do this)

Did you mean show this only the first XX times, then stop? Would that be per-field? If not, the user might not discover all the places where the history feature exists. And even if done per-field they'd then lose the indication that there is some history for a particular field.

[gjsjohnmurray]

Tested this again today and I did noticed that when you have focus on an input, search, then clear your input, that the placeholder doesn't get updated until you move focus away. Wonder if we're able to account for this state? It feels more like a polish item so no worries if it's too complicated.

@misolori I have pushed a fix for this.

[jrieken]

How does this work/look with customized keybindings for these?

[gjsjohnmurray]

How does this work/look with customized keybindings for these?

@jrieken at the moment I'm not doing anything to detect custom keybindings. The way I see it, the latest iteration's placeholder suffix represents "there's history you can go back and forth through" rather than "use cursor up and cursor down". A user who has explicitly overridden the default keybinding surely knows about the history feature and knows what keystrokes they should be using to access it.

[miguelsolorio]

Thanks for the update. Not sure the arrows are explicit enough to tell what it is, though I do like those arrows as opposed to the combined one. ↑↓ history does sound a bit weird to me, even though we did get that feedback. I think ↑↓ for history will work just fine.

Did you mean show this only the first XX times, then stop? Would that be per-field? If not, the user might not discover all the places where the history feature exists. And even if done per-field they'd then lose the indication that there is some history for a particular field.

That's a good point, not sure what we'd do in those cases. Maybe for now since we don't do this anywhere else we can skip this one. You have a solid point that someone may not realize all the other places that there is history.

[gjsjohnmurray]

@misolori would you like me to push a change that reinstates the for history suffix to the hint? Shall I also revert to using (...) instead of [...]? Personally I prefer the simple [↑↓] and don't think it's too obscure to be helpful, but I'll defer to your UX expertise.

Perhaps we should consider an A/B experiment?

[miguelsolorio]

For now let's go with (↑↓ for history) as the text placeholder

In the cases where there is other text in the placeholder let's go with:

• e.g. *.ts, src/**/exclude (↑↓ for history) for exclude
• Filter (e.g. *.ts, src/**/exclude) or ↑↓ for history for filter

[gjsjohnmurray]

Maybe @roblourens can help with figuring out if we can get the keybindings from the HistoryInputBox.

Getting help with this is a blocker on @jrieken's point about the hint adapting if a user has different bindings for history.showNext or history.showPrevious

[gjsjohnmurray]

Pushed some changes so that the history hint suffix now only displays if the user hasn't overridden the default keybindings for either of the commands. There's probably a more elegant way of getting the same result, so I'm open to feedback.

[gjsjohnmurray]

Can this be merged so it gets some exercise in Insiders before endgame?

[roblourens]

Let's try it out in Insiders. Thanks for the ping