search

argoproj / argo-ui

Argoproj shared React components
Apache License 2.0
220 stars 178 forks source link

feat(autocomplete): make `autoHighlight` configurable. Fixes #559 #560

Closed Adrien-D closed 3 months ago

Adrien-D commented 3 months ago

Fixes #559

Simply add the autoHighlight to props to still be customisable

Adrien-D commented 3 months ago

@agilgur5 can't really tell if all usage of this component are intend to use autoHighlight, but by passing the props.autoHighlight down to ReactAutocomplete, all the existing components will pass undefined, so it will take the true upstream value, so it shouldn't change the actual behavior and be breaking change. But if you consider that we can't trust this upstream value I can keep this on our side.

agilgur5 commented 3 months ago

can't really tell if all usage of this component are intend to use autoHighlight

For reference, I've looked at all usage of components in Workflows, CD, and Rollouts when making changes (esp breaking ones), such as in #535 -- see my PR description there which lists all usage of it. (technically there can be other consumers too, but it doesn't have stable releases so nbd about those)

all the existing components will pass undefined, so it will take the true upstream value, so it shouldn't change the actual behavior and be breaking change.

Oh, you're correct. Then your PR title is not quite accurate and should be reworded; i.e. "feat(autocomplete): make autoHighlight configurable".

But if you consider that we can't trust this upstream value I can keep this on our side.

Well the react-autocomplete library hasn't had a commit in 6+ years and was archived ~4 years ago, so I don't think it's changing any time soon 😅

But having the default visible here helps with typings; it will show the default whereas without it, it's ambiguous to a consumer since react-autocomplete's usage is encapsulated.

Technically speaking, we should use the exact same type directly, i.e. with ReactAutocomplete.Props['autoHighlight'], except it turns out that the typings are missing the default.