[feat][Kibana Presentation] Options List new feature #149121

logeekal · 2023-01-18T13:40:39Z

Background

Security solution recently started using controls plugin to provide users some extra filtering capability with help of options list control.

During this implementation, there was some feedback was given from design team + we were facing some minor issues because of the caches. Below section gives the list of changes and the reasoning behind each change. All of these changes were discussed with @ThomThomson

Summary

This PR introduces 3 new functionalities for optionsList embeddables.

Cache invalidation option when reloading an optionsList
- In security solution we have transactional alerting system, where user frequently update alerts data
- We need the latest data and 1 minute cache of optionsList was preventing us from doing so.
- This change adds the capability to clear the cache from an embeddable.
option to hideSearch Panel
- As a client of control plugin, we look some control over what panels are visible and what are not.
Option to add custom placeholder for optionsList
- Out product team felt that default placeholder for optionsList Any may be confusing for the user and hence I have added the option for clients to provide a placeholder.

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2023-01-18T13:41:32Z

Pinging @elastic/kibana-presentation (Team:Presentation)

Heenawter

Code review more-or-less LGTM! Looking for some extra context on why some of this is necessary, though - I'm cautious of just blindly approving the addition of features without understanding why they were added in the first place.

Will also run this locally - since stuff was added to the explicitInput, I just want to double check that this doesn't cause any weirdness with the unsaved changes on the dashboard. It shouldn't, since these values are never actually set, I just want to be extra cautious since this has been a major pain point in the past.

Heenawter · 2023-01-18T15:50:35Z

src/plugins/controls/public/options_list/components/options_list_popover.tsx

      style={{ width: width > 300 ? width : undefined }}
      data-test-subj={`optionsList-control-popover`}
      aria-label={OptionsListStrings.popover.getAriaLabel(fieldName)}
    >
      <EuiPopoverTitle paddingSize="s">{title}</EuiPopoverTitle>
-      {field?.type !== 'boolean' && (
+      {field?.type !== 'boolean' && !hideSearch && (


This won't just hide the search bar, it will hide the entire top action bar, which currently includes the "Show only selections", "Clear selections", and "Sort" functionality:

If this is the desired behaviour, that's fine! But in that case, I would recommend titling this property hideActionBar or something more specific to make this as clear as possible.

@Heenawter , you are right and I agree with changing the name to hideActionBar.

I was thinking if it makes sense to have a control to toggle the visibility of search bar. Do we want to go for a scenario where search panel is not visible and rest of 3 controls are visible. I guess that might seem weird. What do you think?

At least we do not have that requirement and I am good renaming it to hideActionBar

I think hiding the entire top action bar makes sense to me! :) I assume if this is being done, the solution knows that there are < 10 options to select from. So searching, sorting, etc. shouldn't be necessary in these cases, since the options list size is manageable without them 👍

src/plugins/controls/public/options_list/embeddable/options_list_embeddable.tsx

Heenawter · 2023-01-18T15:57:06Z

src/plugins/controls/public/options_list/components/options_list_popover.tsx

@@ -52,12 +53,13 @@ export const OptionsListPopover = ({
  return (
    <div
      id={`control-popover-${id}`}
+      className={`optionsList__popover`}


Why the addition of this extra class? Is the style attribute below not covering this?

I'm also suspicious that the scss import in the definition of this class may have contributed to the +10kb increase in async chunks, which is a bit worrying.... Not sure about that, though - just a guess :)

@Heenawter not it was not necessary, since there already was scss file so I just added the style in that.

also, I will check about bundle size increase if it was because of scss. But I just wanted to have consistency in the width.

@logeekal I guess I'm still a bit confused. Right below this, we have:

style={{ width: width > 300 ? width : undefined }}

I think if we are going the route of styling through a class, then we should remove this - styling through both a class name and a style attribute for a single component can cause two sources of truth, which is hard to follow imo. For example, after this addition, is the minimum width of the popover 300px or is it $controlMinWidth (which is actually a variable meant to reference the size of the control and not the popover)?

For reference, this is the way it works currently for a small control:

You are right and it was my fault I did not pay attention to the style prop. Previously, if width < 300, then we were setting width as undefined, not 300. So now I have changed min-width to 300px. Let me know if it make sense now.

I don't think the new min-width is working quite as expected, either. Here's what happens when you shrink the window to be less than 300px wide with your current code:

Notice that the white part of the popover shrinks, but neither the inner contents nor the footer do - so you end up with a weird "overhang" effect. Whereas this is what happens with the original sizing code:

The sizing of the popovers is finicky! So we don't actually technically have a minimum size - we are allowing the popover to decide the minimum size, hence setting the width to undefined when the width of the control is less than 300. Basically, the original code says "If the width of the control is <= 300, then let the popover be in control of its own width; otherwise, set the popover width to be the same size as the control width."

If you can figure out a better way to handle this, I'd be down - but in its current state, not a fan of what happens when you set min-width like that.

Thanks @Heenawter for diving into this problem. However, I think it might not be that big of a problem. Please bear with me for the explanation.

It looks like there are 4 ways we can go about this problem:

Setting width to undefined if width < 300 i.e. width: width > 300 ? width : undefined

This is the original definition and it makes the popover look as below if we remove ActionBar so I guess we will need to have width for sure.

Not setting min-width and setting width to the size of option list. Issue with this is that, option list can get very small in some cases. See below:

Because of these 2 reasons, I think setting min-width becomes important. Now one option is to set the min-width of the popover content as done above i.e min-width : 300px

I think this works well in our case because screen width will never( rarely?) be <300 causing the issue that you just highlighted.

If you check EuiTour component, it also suffers from the same problem. I will raise an issue with them.

Solution to the problem in the 3rd approach is passing panelStyle to EuiPopover.

This solves above problem but it may give rise to other positioning problems mentioned here.

Although I have not encountered any issue in our case so I am good with this approach as well but I prefer 3rd approach more.

Ahh, thank you for the screenshots! That helps a lot to understand why you added that - makes total sense. I did not consider how the width would be impacted when the action bar was not present. I think that, given what you've outlined, sticking with your current min-width approach seems totally fair!

🙏 Thank you very much.

src/plugins/controls/public/options_list/embeddable/options_list_embeddable.tsx

logeekal · 2023-01-18T17:12:19Z

Code review more-or-less LGTM! Looking for some extra context on why some of this is necessary, though - I'm cautious of just blindly approving the addition of features without understanding why they were added in the first place.

@Heenawter , Thank you very much for taking a look at the PR. I have added detailed context in the description.

ThomThomson

I'm often wary of additions that inject new functionality into the embeddable plugin which isn't universal. I would consider this to be leaking controls implementation details into any plugin that uses Embeddables.

Rather than adding this boolean into the generic embeddable reload function, I would prefer either of these alternate approaches:

You could add a new method on the options_list embeddable - or even at the control_group level that explicitly clears the cache? Then in your implementation you could call controlGroup.clearCache(); controlGroup.reload()
Make the options list control's reload function always clear the cache. This makes sense to me because if the user is specifically asking for the controls to reload, they likely want it to actually hit the server, rather than just getting cached data.

If you use either of these methods, we can avoid modifying the embeddable plugin / interfaces.

logeekal · 2023-01-22T19:02:49Z

Make the options list control's reload function always clear the cache. This makes sense to me because if the user is specifically asking for the controls to reload, they likely want it to actually hit the server, rather than just getting cached data.

I am also inclined towards this approach and have implemented it. reload function will clear the cache by default.

kibana-ci · 2023-01-22T22:40:55Z

💚 Build Succeeded

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`	260	264	+4

Async chunks

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

id	before	after	diff
`controls`	171.0KB	171.2KB	+140.0B

Page load bundle

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

id	before	after	diff
`controls`	31.5KB	31.6KB	+98.0B

Unknown metric groups

API count

id	before	after	diff
`controls`	264	268	+4

History

💔 Build #101646 failed d802860
💚 Build #101034 succeeded c9400ba

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

ThomThomson

Changing my review status because the changes to the Embeddable plugin have been reverted! Thanks for doing that.

I would still consider this pending on @Heenawter's recent feedback.

Heenawter

The design concerns I noticed have all been addressed 👍 Thanks for the contribution!

logeekal added 2 commits January 18, 2023 14:01

feat(options): hideSearch/placeholder/cache invalidate

1273c60

fix type issues

c9400ba

logeekal requested review from a team as code owners January 18, 2023 13:40

botelastic bot added the Feature:Embedding Embedding content via iFrame label Jan 18, 2023

logeekal added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Jan 18, 2023

logeekal added the release_note:skip Skip the PR/issue when compiling release notes label Jan 18, 2023

Heenawter reviewed Jan 18, 2023

View reviewed changes

ThomThomson requested changes Jan 18, 2023

View reviewed changes

logeekal added 3 commits January 22, 2023 19:09

Reset embedable interface + reload clears cache

9221eae

rename hideSearch to hideActionBar

4474eab

fix width

421b8ef

logeekal added 3 commits January 22, 2023 20:11

fix min width of popover

88ddd8e

Merge branch 'main' into feat/control_plugin_options_feats

d802860

refactor: test correction

5e3a964

ThomThomson approved these changes Jan 23, 2023

View reviewed changes

Heenawter approved these changes Jan 24, 2023

View reviewed changes

logeekal removed the request for review from a team January 24, 2023 17:09

logeekal merged commit 18aff79 into elastic:main Jan 24, 2023

kibanamachine added v8.7.0 backport:skip This commit does not require backporting labels Jan 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat][Kibana Presentation] Options List new feature #149121

[feat][Kibana Presentation] Options List new feature #149121

logeekal commented Jan 18, 2023 •

edited

Loading

elasticmachine commented Jan 18, 2023

Heenawter left a comment

Heenawter Jan 18, 2023 •

edited

Loading

logeekal Jan 18, 2023

Heenawter Jan 18, 2023

logeekal Jan 22, 2023

Heenawter Jan 18, 2023 •

edited

Loading

logeekal Jan 18, 2023 •

edited

Loading

Heenawter Jan 18, 2023 •

edited

Loading

logeekal Jan 22, 2023

Heenawter Jan 23, 2023 •

edited

Loading

logeekal Jan 24, 2023

Heenawter Jan 24, 2023

logeekal Jan 24, 2023

logeekal commented Jan 18, 2023

ThomThomson left a comment

logeekal commented Jan 22, 2023

kibana-ci commented Jan 22, 2023

API count

ThomThomson left a comment

Heenawter left a comment

[feat][Kibana Presentation] Options List new feature #149121

[feat][Kibana Presentation] Options List new feature #149121

Conversation

logeekal commented Jan 18, 2023 • edited Loading

Background

Summary

Checklist

For maintainers

elasticmachine commented Jan 18, 2023

Heenawter left a comment

Choose a reason for hiding this comment

Heenawter Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

logeekal Jan 18, 2023

Choose a reason for hiding this comment

Heenawter Jan 18, 2023

Choose a reason for hiding this comment

logeekal Jan 22, 2023

Choose a reason for hiding this comment

Heenawter Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

logeekal Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Heenawter Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

logeekal Jan 22, 2023

Choose a reason for hiding this comment

Heenawter Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

logeekal Jan 24, 2023

Choose a reason for hiding this comment

Heenawter Jan 24, 2023

Choose a reason for hiding this comment

logeekal Jan 24, 2023

Choose a reason for hiding this comment

logeekal commented Jan 18, 2023

ThomThomson left a comment

Choose a reason for hiding this comment

logeekal commented Jan 22, 2023

kibana-ci commented Jan 22, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

ThomThomson left a comment

Choose a reason for hiding this comment

Heenawter left a comment

Choose a reason for hiding this comment

logeekal commented Jan 18, 2023 •

edited

Loading

Heenawter Jan 18, 2023 •

edited

Loading

Heenawter Jan 18, 2023 •

edited

Loading

logeekal Jan 18, 2023 •

edited

Loading

Heenawter Jan 18, 2023 •

edited

Loading

Heenawter Jan 23, 2023 •

edited

Loading