Select: Preserve value when allowCustomValue is set

[ashharrison90]

What is this feature?

• improves some typings in SelectBase
• adds support to <Select> for editing a custom value
  • this only applies when allowCustomValue is true and isMulti is false
  • this works by making the react-select input state controlled by us
    • onMenuOpen we restore the internal input state to be the selected values label
    • onMenuClose we clear the input state else react-select doesn't show the SingleValue component correctly
    • onInputChange we update the internal input value state
• updates some unit tests that relied on the old behaviour

Why do we need this feature?

• so users can edit a custom value once entered

Who is this feature for?

• anyone using adhoc filters!

Which issue(s) does this PR fix?:

Fixes https://github.com/grafana/hyperion-planning/issues/27

Special notes for your reviewer:

Please check that:

• It works as expected from a user's perspective.
• If this is a pre-GA feature, it is behind a feature toggle.
• The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

[ashharrison90]

this test was duplicated below, so removed it

[matyax]

Suggested change

	// we expect 2 elemnts having "Option 2": the input itself and the option.
	// we expect 2 elements having "Option 2": the input itself and the option.

[matyax]

👍 logs changes.

[joshhunt]

I think this is secretly actually a very big an impactful change... it's a pretty big conceptual change to how custom values work and users interact with them. In saying that, I'm pretty supportive of this - this half-answers a question about fixing this behaviour in the new select component.

Two issues I spotted with it:

When editing a value, trying to click to place the cursor doesn't work. It just closes the dropdown. The only way to move the cursor is to either backspace or use arrow keys.

I think it's only used in plugins, but conincidentally I noticed a style regression with multi select with the noMultiValueWrap prop enabled.

https://developers.grafana.com/ui/canary/index.html?path=/story/forms-select--multi-plain-value&args=noMultiValueWrap:!true

vs

http://localhost:9001/?path=/story/forms-select--multi-plain-value&args=noMultiValueWrap:!true

[ashharrison90]

When editing a value, trying to click to place the cursor doesn't work. It just closes the dropdown. The only way to move the cursor is to either backspace or use arrow keys.

yeah i haven't quite found a way to fix that yet. there is one way, but it involves setting the controlShouldRenderValue prop to false, which significantly alters the behaviour for unit tests (basically the SingleValue component is never rendered and we instead rely on the internal <input> showing the value)

I think it's only used in plugins, but conincidentally I noticed a style regression with multi select with the noMultiValueWrap prop enabled.

i see the same behaviour in canary as on this branch? but not on https://developers.grafana.com/ui/latest/index.html?path=/story/forms-select--multi-plain-value&args=noMultiValueWrap:!true

so must be something that's regressed recently. will investigate that separately.

[ashharrison90]

I think it's only used in plugins, but conincidentally I noticed a style regression with multi select with the noMultiValueWrap prop enabled.

yep, it's caused by #87085 😬 fyi @kaydelaney
you can repro by going to the Select -> Multi plain value story, setting noMultiValueWrap to true and selecting options. see https://developers.grafana.com/ui/latest/index.html?path=/story/forms-select--multi-plain-value for what it should look like

[ashharrison90]

When editing a value, trying to click to place the cursor doesn't work. It just closes the dropdown. The only way to move the cursor is to either backspace or use arrow keys.

ok, dug a bit more into it this morning. it's caused by the menuShouldBlockScroll option passed to react-select. here's some options, roughly in order of preference:

• do nothing. it's the current behaviour of Select anyway. i guess this exposes the problem a little more, but still 🤷
• set menuShouldBlockScroll=false for Selects that allow custom values. downside is it introduces an inconsistency between different types of Select where one can scroll the page whilst the menu is open and the other can't. and it introduces the downsides explained below for those Selects
• set menuShouldBlockScroll=false for all Selects. this can cause some slightly weird issues where you scroll a select outside of the scroll container but the menu is still visible and overlaying everything, e.g. panel edit:
  

[dprokop]

@ashharrison90 i think it's ok to "do nothing. it's the current behaviour of Select anyway. i guess this exposes the problem a little more, but still 🤷" :) I mean - i personally did not find this problematic, thinking about the following proposals, they bring to much of a change to the UX imo.

[joshhunt]

I would have hoped we could listen to the click even on the input, handle it there, and prevent it from propagating up to the rest of react-select?

Otherwise, I think it's fine (but not ideal) to leave the behaviour as-is then. I think this highlights the existing awkward behaviour even more, but it's probably fine to keep it around for a bit more. 👍

[joshhunt]

also good catch on the weird display issue - when I spotted it it wasn't on canary yet, but I see it now is 👍