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

[Controls] Allow wildcard searching in options list #158427

Merged

Conversation

Heenawter
Copy link
Contributor

@Heenawter Heenawter commented May 24, 2023

Closes #157157
Closes #152921

Summary

The primary goal of this PR is to introduce an option for wildcard ("contains") searching to the options list control:

Screen.Recording.2023-05-29.at.8.41.46.AM.mov

New Flyout Design

However, since this required adding a radio group to the custom options list settings in the create/edit control flyout, I also made some changes to the flyout design in order to better accommodate this (we were previously using EuiSwitch components for the control-type-specific settings, which did not work in this case because I wanted to be able to add a tooltip to describe each search type):

Before After
May-29-2023 09-04-02 May-29-2023 09-00-21

Note in the above GIFs that, since I was using an EuiRadioGroup for the search technique setting, I decided it made more sense + was more consistent for the "Allow multiple selections in dropdown" to also be converted to a radio group rather than a switch. The "Ignore timeout for results" setting is the only one that remained a switch in the new design:

Before After
image image

EuiSwitch with Tooltip Bug

As part of this redesign, I also fixed a very quick bug where, because the old SwitchWithTooltip was defined inside the larger OptionsListEditorOptions component, any state update on OptionsListEditorOptions would cause SwitchWithTooltip to also be re-rendered - this caused the "slide" animation to be interrupted on click:

Before After
image image

Title Bug Fix

And, since I was making so many changes to the flyout code as part of refactoring the code (including the design changes above), I also fixed a bug with control titles where things weren't getting set properly. To test this, consider taking the following steps:

  1. Create a new options list control, keeping the default title
  2. Edit that options list control and change it to a range slider control by selecting a number field
  3. Notice that...
    • Before this PR, the title gets completely cleared:
      image
    • After this PR, the default title gets updated to the range slider field name:
      image
  4. Delete that range slider control and create a new control, keeping the default title once again (options list or range slider, the type doesn't matter).
  5. Edit the control and change the field to a different field of the same type (i.e. if your control from step 4 was an options list control, select a field that keeps it as an options list control). Before saving your changes, notice that the "default title" in the Label input gets updated to the new field title:


  6. After saving, notice that...
    • Before this PR, the title doesn't actually get updated to the new default title:
      image
    • After this PR, the title gets updated as expected:
      image

Flaky Test Runner

test/functional/apps/dashboard_elements/controls/options_list/options_list_suggestions.ts:

Checklist

For maintainers

@Heenawter Heenawter added release_note:enhancement Feature:Input Control Input controls visualization Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:medium Medium Level of Effort impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. backport:skip This commit does not require backporting Project:Controls v8.9.0 labels May 24, 2023
@Heenawter Heenawter force-pushed the options-list-wildcard-query_2023-05-24 branch from 57dc9ba to 837eabc Compare May 25, 2023 16:11
@Heenawter Heenawter force-pushed the options-list-wildcard-query_2023-05-24 branch 2 times, most recently from d429419 to abefc72 Compare May 26, 2023 15:19
@Heenawter Heenawter force-pushed the options-list-wildcard-query_2023-05-24 branch from abefc72 to 4eb520b Compare May 26, 2023 15:19
@Heenawter Heenawter force-pushed the options-list-wildcard-query_2023-05-24 branch from 256c251 to 2468414 Compare May 26, 2023 21:22
@Heenawter Heenawter force-pushed the options-list-wildcard-query_2023-05-24 branch from 90405c3 to f7b6226 Compare May 29, 2023 14:15
@Heenawter Heenawter force-pushed the options-list-wildcard-query_2023-05-24 branch from 453524a to e4671ec Compare May 29, 2023 16:43
@Heenawter Heenawter added loe:large Large Level of Effort and removed loe:medium Medium Level of Effort labels Jun 1, 2023
@jsanz
Copy link
Member

jsanz commented Jun 2, 2023

Very nice additions and fixes @Heenawter 👏, the new flyout makes much more sense, I love it.

One question: would it make sense to somehow indicate in the main interface which search option is enabled? I'm thinking in final users that likely can't access the editing UI. Not a strong opinion either way because the behaviour is quite self-explanatory anyways. What do you think?

Copy link
Contributor

@florent-leborgne florent-leborgne left a comment

Choose a reason for hiding this comment

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

Hi! Copy LGTM, I left a few very minor copy suggestions on behalf of @elastic/ux-writers.
I've been wondering one thing though: since it's possible to choose between "starts with" and "contains", would it make sense to show which mode is enabled on the search field itself?

Copy link
Contributor

@florent-leborgne florent-leborgne left a comment

Choose a reason for hiding this comment

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

Hi! Copy LGTM, I left a few very minor copy suggestions on behalf of @elastic/ux-writers.

@Heenawter
Copy link
Contributor Author

Heenawter commented Jun 2, 2023

@florent-leborgne I've been wondering one thing though: since it's possible to choose between "starts with" and "contains", would it make sense to show which mode is enabled on the search field itself?

@jsanz would it make sense to somehow indicate in the main interface which search option is enabled? I'm thinking in final users that likely can't access the editing UI.

These are very, very good points. @andreadelrio, any idea how we might indicate which search method is being used in the UI? I looked at potentially using an icon, but I couldn't find any that feel like a good fit...

Changing the placeholder for the search box seems like a good route, but I'm not really sure what would make the most sense for that - would "Search for a value by prefix" and "Search for a value by substring" be clear to a less technical end user? Or could it maybe be changed to something as simple as "Starts with" versus "Contains" - would that be clear enough that it is performing a search with these different techniques? @florent-leborgne Your input would probably be helpful here 🤔

@florent-leborgne
Copy link
Contributor

@Heenawter, the placeholder for the search box sounds good to me.

After years of using Jira at different companies, I got used to the way they do it
image

So what you suggested sounds kind of natural to me: "Starts with" and "Contains". Not a fan of ellipsis in search fields but this is one case where it wouldn't bug me too much as those sound like starting a sentence "Starts with..." - "Contains..." No strong opinion here.

@Heenawter
Copy link
Contributor Author

@florent-leborgne Yes, I think I like changing the placeholder to "Starts with..." or"Contains..." depending on the search technique - it feels pretty natural to me :)

Screen.Recording.2023-06-02.at.9.57.06.AM.mov

@andreadelrio @jsanz @ThomThomson Thoughts on this?

@ThomThomson
Copy link
Contributor

Oh yes the Starts with and Contains idea looks great, and is a great way to communicate the search strategy! Excellent suggestion @jsanz and good idea for the placement @Heenawter @florent-leborgne!

@andreadelrio
Copy link
Contributor

@florent-leborgne Yes, I think I like changing the placeholder to "Starts with..." or"Contains..." depending on the search technique - it feels pretty natural to me :)

Screen.Recording.2023-06-02.at.9.57.06.AM.mov
@andreadelrio @jsanz @ThomThomson Thoughts on this?

I think this works great! and it's an existing pattern in other apps.

@jsanz
Copy link
Member

jsanz commented Jun 2, 2023

@andreadelrio @jsanz @ThomThomson Thoughts on this?

Very neat, compact, and totally "out of the way", nice!! 💯

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

This is looking very clean. In addition to fixing the title bug, you've also fixed the issue where canceling out of the control form would give you a warning even if you had no changes. Nice!

Excellent functional and jest test additions. Tested this locally (thanks for fixing the expand width bug so quickly) and by looking through the code and everything LGTM! Left a couple small nits.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #36 / discover discover field visualize button should visualize correctly text based language queries in Lens

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
controls 297 299 +2

Async chunks

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

id before after diff
controls 188.9KB 190.9KB +2.0KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
controls 14 15 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
controls 33.0KB 36.8KB +3.8KB
Unknown metric groups

API count

id before after diff
controls 304 306 +2

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 414 418 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 498 502 +4
total +6

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@Heenawter Heenawter merged commit 8277665 into elastic:main Jun 6, 2023
17 checks passed
@Heenawter Heenawter deleted the options-list-wildcard-query_2023-05-24 branch June 6, 2023 14:32
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 Feature:Input Control Input controls visualization impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:large Large Level of Effort Project:Controls release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas ui-copy Review of UI copy with docs team is recommended v8.9.0
Projects
None yet
8 participants