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

[ES3][Connectors]Using EuiComboBox with searchable capability as we do in ESS #199464

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

JoseLuisGJ
Copy link
Contributor

@JoseLuisGJ JoseLuisGJ commented Nov 8, 2024

Summary

This PR swaps the EuiSelectable by the EuiComboBox component as we have in ESS.

Context:

This PR belongs to this initiative https://github.com/elastic/search-team/issues/8000 where we agreed on bring low hanging fruit artefacts from ESS to ES3 before being replace completly with the full new experience which will be later. Therefor we are not investing effort on make the code scalable and reusable at this point.

CleanShot 2024-11-08 at 12 59 47

@JoseLuisGJ JoseLuisGJ changed the title Using EuiComboBox with serchable capability as we do in ESS [ES3][Connectors]Using EuiComboBox with searchable capability as we do in ESS Nov 8, 2024
@JoseLuisGJ JoseLuisGJ added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:Search v9.0.0 labels Nov 8, 2024
@JoseLuisGJ JoseLuisGJ marked this pull request as ready for review November 8, 2024 12:14
@JoseLuisGJ JoseLuisGJ requested a review from a team as a code owner November 8, 2024 12:14
@JoseLuisGJ
Copy link
Contributor Author

@elasticmachine merge upstream

@JoseLuisGJ
Copy link
Contributor Author

@elasticmachine merge upstream

// We only want to allow people to set the service type once to avoid weird conflicts
isDisabled={Boolean(connector.service_type) || isDisabled}
isLoading={isLoading}
data-test-subj="serverlessSearchEditConnectorTypeChoices"
Copy link
Contributor

@TattdCodeMonkey TattdCodeMonkey Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine, but we should watch out for this change breaking the connectors synthetic tests.

mutate(allConnectors[keySelected].serviceType);
}}
renderOption={renderOption}
rowHeight={(euiTheme.base / 2) * 5}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick, none of the stock sizes are close to what we need?

Copy link
Contributor Author

@JoseLuisGJ JoseLuisGJ Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, I tried back and forth but this is the most accurate value we should use

contentClassName: string
) => {
const { _append, key, label, _prepend, serviceType } =
option as EuiComboBoxOptionOption<OptionData> & {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ fix the type instead of casting

type ExpandedComboBoxOption = EuiComboBoxOptionOption<OptionData> & {
  _append: JSX.Element[];
  _prepend: JSX.Element;
  serviceType: string;
};

const renderOption = (
    option: ExpandedComboBoxOption,
    searchValue: string,
    contentClassName: string
  ) => {

Or you can just add _append, _prepend, serviceType to OptionData

Copy link
Contributor Author

@JoseLuisGJ JoseLuisGJ Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I add _append, _prepend, serviceType to OptionData :

interface OptionData {
  secondaryContent?: string;
  _append: React.ReactNode[];
  _prepend: React.ReactNode;
  serviceType: string;
}

I run in this error: Property '_append' does not exist on type 'EuiComboBoxOptionOption<OptionData>

When:

 ) => {
    const { _append, key, label, _prepend, serviceType } =
      option as EuiComboBoxOptionOption<OptionData>;
    return (

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I fix the type. I get this error:

No overload matches this call.
  Overload 1 of 2, '(props: _EuiComboBoxProps<OptionData>): EuiComboBox<OptionData>', gave the following error.
    Type '(option: ExpandedComboBoxOption, searchValue: string, contentClassName: string) => React.JSX.Element' is not assignable to type '(option: EuiComboBoxOptionOption<OptionData>, searchValue: string, OPTION_CONTENT_CLASSNAME: string) => ReactNode'.
      Types of parameters 'option' and 'option' are incompatible.
        Type 'EuiComboBoxOptionOption<OptionData>' is not assignable to type 'ExpandedComboBoxOption'.
          Type 'EuiComboBoxOptionOption<OptionData>' is missing the following properties from type '{ _append: Element[]; _prepend: Element; serviceType: string; }': _append, _prepend, serviceType
  Overload 2 of 2, '(props: _EuiComboBoxProps<OptionData>, context: any): EuiComboBox<OptionData>', gave the following error.
    Type '(option: ExpandedComboBoxOption, searchValue: string, contentClassName: string) => React.JSX.Element' is not assignable to type '(option: EuiComboBoxOptionOption<OptionData>, searchValue: string, OPTION_CONTENT_CLASSNAME: string) => ReactNode'.ts(2769)

Consuming the render options:

<EuiComboBox
   ...
   renderOption={renderOption}
   ...
/>

)}
options={initialOptions}
selectedOptions={initialOptions.filter(
(option) => option.serviceType === connector.service_type
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me how to get a unique serviceType from here. I'm getting 3 items for any Jira connector selected. @sphilipse how could I apply your suggestion array.filter((item,index) => array.indexOf(item) === index)); here?

CleanShot 2024-11-13 at 15 20 55@2x

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First you have to decide what you actually want to display there. All three of these options have the same service type, because you are selecting a service type that can handle all three of these options. So maybe it's okay to display all three?

Copy link
Contributor Author

@JoseLuisGJ JoseLuisGJ Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, we should just display 1 Jira item in the dropdown IMO not 3. That would be more accurate and less confusing.

CleanShot 2024-11-13 at 15 41 40@2x

cc: @danajuratoni @seanstory

Currently I realised this serviceType issue is also affecting the current experience in ES3 if you watch the video choosing any Confluence (either Cloud & Server or Data Center) connector always applies the Confluence Cloud & Server in the table and the same happens choosing any Jira (either Cloud, Server or Data Center) connector it always applies the Jira Cloud

CleanShot.2024-11-13.at.14.20.54.mp4

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, we should just display 1 Jira item in the dropdown IMO not 3. That would be more accurate and less confusing.

I'm fine with that. I think that's how we did confluence and github in ESS, where you pick which "flavor" you want in the configuration tab, not from the tiles at creation time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what we display now in ESS for Confluence:

CleanShot 2024-11-14 at 11 46 10@2x

And this for Github:

CleanShot 2024-11-14 at 11 47 25@2x

Copy link
Contributor Author

@JoseLuisGJ JoseLuisGJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All done. Thanks @TattdCodeMonkey

@JoseLuisGJ
Copy link
Contributor Author

@elasticmachine merge upstream

@JoseLuisGJ
Copy link
Contributor Author

@elasticmachine merge upstream

@JoseLuisGJ
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
serverlessSearch 366.4KB 368.0KB +1.6KB

History

@JoseLuisGJ
Copy link
Contributor Author

@elasticmachine merge upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Search v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants