feat: add DebounceSegment element

[kminehart]

The Segment element is basically an Input with options. Unlike the Select option, it's value is a string and does not depend on an "option" being available in the "options" that matches that "value".

[scottlepp]

This seems like a feature missing from Segment? If we add new features, are we going to add new components each time? So, it seems we just call it Segment (and our "Segment" component just has more features that may eventually go into core grafana)

[kminehart]

I don't know if "debouncing" is a feature that Grafana wants in their UI library?

I'll be honest though, a big reason for me not wanting to put it into @grafana/ui, even if it may belong there, is that it'll break the component for anyone not using 7.4.0+. If this repo didn't exist, then this would just be another component in the servicenow-datasource repository.

[scottlepp]

Regardless of whether it goes to grafana ui, it just seems like another option that we can add to our "Segment" (instead of calling it DebounceSegment). We can keep enhancing Segment, rather than creating a new component for every new feature we add to Segment.

[vickyyyyyyy]

LGTM, just left some nit comments.

I also noticed we are using querySelector('a') in the tests, is it not possible to use getByDisplayText / getByText?

I agree with @scottlepp that we can add this in the Segment component in @grafana/ui - something to ask the frontend team/UX to see if this belongs there/if they want this feature. I can do this because I've been in discussions with Luke and Alex on Enterprise components vs OSS vs where they belong.

Also agreeing with @kminehart (repeating his words but longer), if we are adding this into @grafana/ui, it will be a while before it gets reviewed, merged and released. And even after it is released, we won't be able to use it in our plugins without bumping up the required Grafana version needed for them (ServiceNow atm is 7.0.x so even if this is released asap, we would need to bump it to 7.4.1+) which I'm guessing we don't want to do because existing clients most likely take a while before they update their whole Grafana version (probably something to ask @mjseaman)?

Regarding the naming, this wasn't something I've thought about previously (when I added DebounceInput which is probably what this PR is based on, and why it was named this way) and I agree, I think it does make sense to name it Segment.

My thoughts for why this would be good is that we can save on the imports and repeated code that wraps OSS Segment for every feature we want. Also, if we decide to move new features into OSS, without the renaming, we would need to refactor:

- import { DebounceSegment } from "ui-enterprise"
- import { AnotherSegment } from "ui-enterprise"
- import { YetAnotherSegment } from "ui-enterprise"
import { Segment } from "@grafana/ui"

<Component>
   // we are using the OSS Segment here
   <Segment />
   ...
   // we now want to use DebounceSegment here
-  <DebounceSegment />
+  <Segment />
   ...
   // we now want to use AnotherSegment here
-  <AnotherSegment />
+  <Segment />
   ...
   // we now want to use YetAnotherSegment here
-  <YetAnotherSegment />
+  <Segment />
   ...
</Component>

vs renaming and having one Segment component here with multiple features:

- import { Segment as EnterpriseSegment } from "ui-enterprise"
import { Segment } from "@grafana/ui"

<Component>
   // we are using the OSS Segment here
   <Segment />
   ...
   // we now want to use DebounceSegment here
-  <EnterpriseSegment />
+  <Segment />
   ...
   // we now want to use AnotherSegment here
-  <EnterpriseSegment />
+  <Segment />
   ...
   // we now want to use YetAnotherSegment here
-  <EnterpriseSegment />
+  <Segment />
   ...
</Component>

What do you think? Let me know if these were along the lines of both your thoughts' too or if I've misunderstood.

TLDR;
I agree with both Scott and Kevin, and I brain dumped my thoughts which are basically repeating what you both said but in more words 😄

[vickyyyyyyy]

is there a reason these aren't combined in the same useEffect? If it's because this was copying the DebounceInput, that probably should have been combined too lol (not saying the change for the other component needs to be done in this PR)

[kminehart]

Yeah, because that would look like this:

  React.useEffect(() => {
      onDebounce(debouncedSegment);
      setInput(value);
}, [value, debouncedSegment]);

Technically that wouldn't be the same thing, since it'd call onDebounce every time value is updated, and would call setInput when debouncedSegment is updated.

[kminehart]

That makes sense to me, @scottlepp @vickyyyyyyy . I'll rename it to Segment

Sorry @scottlepp I completely misunderstood your first comment. 😅

[vickyyyyyyy]

LGTM

[scottlepp]

lgtm