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.

Sign up for GitHub

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

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

Merged
merged 8 commits into from
Jan 24, 2023
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1273c60
feat(options): hideSearch/placeholder/cache invalidate
logeekal Jan 18, 2023
c9400ba
fix type issues
logeekal Jan 18, 2023
9221eae
Reset embedable interface + reload clears cache
logeekal Jan 22, 2023
4474eab
rename hideSearch to hideActionBar
logeekal Jan 22, 2023
421b8ef
fix width
logeekal Jan 22, 2023
88ddd8e
fix min width of popover
logeekal Jan 22, 2023
d802860
Merge branch 'main' into feat/control_plugin_options_feats
logeekal Jan 22, 2023
5e3a964
refactor: test correction
logeekal Jan 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/plugins/controls/common/options_list/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ export interface OptionsListEmbeddableInput extends DataControlInput {
hideExclude?: boolean;
hideExists?: boolean;
hideSort?: boolean;
hideSearch?: boolean;
exclude?: boolean;
placeholder?: string;
}

export type OptionsListField = FieldSpec & {
Expand Down
6 changes: 6 additions & 0 deletions src/plugins/controls/public/options_list/components/options_list.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
@import '../../control_group/control_group.scss';

.optionsList__anchorOverride {
display:block;
}
Expand Down Expand Up @@ -86,3 +88,7 @@
.optionsList--sortPopover {
width: $euiSizeXL * 7;
}

.optionsList__popover {
min-width: $controlMinWidth;
}
4 changes: 3 additions & 1 deletion src/plugins/controls/public/options_list/components/options_list_control.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ export const OptionsListControl = ({ typeaheadSubject }: { typeaheadSubject: Sub
const exclude = select((state) => state.explicitInput.exclude);
const id = select((state) => state.explicitInput.id);

const placeholder = select((state) => state.explicitInput.placeholder);

const loading = select((state) => state.output.loading);

// debounce loading state so loading doesn't flash when user types
Expand Down Expand Up @@ -128,7 +130,7 @@ export const OptionsListControl = ({ typeaheadSubject }: { typeaheadSubject: Sub
>
{hasSelections || existsSelected
? selectionDisplayNode
: OptionsListStrings.control.getPlaceholder()}
: placeholder ?? OptionsListStrings.control.getPlaceholder()}
</EuiFilterButton>
</div>
);
Expand Down
4 changes: 3 additions & 1 deletion src/plugins/controls/public/options_list/components/options_list_popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export const OptionsListPopover = ({
const field = select((state) => state.componentState.field);

const hideExclude = select((state) => state.explicitInput.hideExclude);
const hideSearch = select((state) => state.explicitInput.hideSearch);
const fieldName = select((state) => state.explicitInput.fieldName);
const title = select((state) => state.explicitInput.title);
const id = select((state) => state.explicitInput.id);
Expand All @@ -52,12 +53,13 @@ export const OptionsListPopover = ({
return (
<div
id={`control-popover-${id}`}
className={`optionsList__popover`}
Copy link
Contributor

@Heenawter Heenawter Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

Screenshot 2023-01-18 at 9 13 37 AM

Copy link
Contributor Author

@logeekal logeekal Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@Heenawter Heenawter Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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:

Screenshot 2023-01-18 at 10 54 37 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@Heenawter Heenawter Jan 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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:

Screenshot 2023-01-23 at 9 53 49 AM

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:

image

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. 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.
    • image

  2. 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:

    • image

  3. 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.

  4. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙏 Thank you very much.

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 && (
Copy link
Contributor

@Heenawter Heenawter Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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:

Screenshot 2023-01-18 at 8 48 13 AM

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

<OptionsListPopoverActionBar
showOnlySelected={showOnlySelected}
setShowOnlySelected={setShowOnlySelected}
Expand Down
8 changes: 7 additions & 1 deletion src/plugins/controls/public/options_list/embeddable/options_list_embeddable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import { OptionsListControl } from '../components/options_list_control';
import { ControlsDataViewsService } from '../../services/data_views/types';
import { ControlsOptionsListService } from '../../services/options_list/types';
import { OptionsListField } from '../../../common/options_list/types';
import { OptionsListStrings } from '../components/options_list_strings';

const diffDataFetchProps = (
last?: OptionsListDataFetchProps,
Expand Down Expand Up @@ -112,6 +113,10 @@ export class OptionsListEmbeddable extends Embeddable<OptionsListEmbeddableInput
this.initialize();
}

public getInputPlaceHolder = () =>
this.reduxEmbeddableTools.getState().explicitInput.placeholder ??
OptionsListStrings.control.getPlaceholder();
Heenawter marked this conversation as resolved.
Show resolved Hide resolved

private initialize = async () => {
const { selectedOptions: initialSelectedOptions } = this.getInput();
if (!initialSelectedOptions) this.setInitializationFinished();
Expand Down Expand Up @@ -405,7 +410,8 @@ export class OptionsListEmbeddable extends Embeddable<OptionsListEmbeddableInput
return [newFilter];
};

reload = () => {
reload = (clearCache: boolean = false) => {
if (clearCache) this.optionsListService.clearOptionsListCache();
Heenawter marked this conversation as resolved.
Show resolved Hide resolved
this.runOptionsListQuery();
};

Expand Down
3 changes: 3 additions & 0 deletions src/plugins/controls/public/services/options_list/options_list.story.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,15 @@ let optionsListRequestMethod = async (request: OptionsListRequest, abortSignal:
)
);

const clearOptionsListCacheMock = () => {};

export const replaceOptionsListMethod = (
newMethod: (request: OptionsListRequest, abortSignal: AbortSignal) => Promise<OptionsListResponse>
) => (optionsListRequestMethod = newMethod);

export const optionsListServiceFactory: OptionsListServiceFactory = () => {
return {
runOptionsListRequest: optionsListRequestMethod,
clearOptionsListCache: clearOptionsListCacheMock,
};
};
4 changes: 4 additions & 0 deletions src/plugins/controls/public/services/options_list/options_list_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ class OptionsListService implements ControlsOptionsListService {
return { rejected: true } as OptionsListResponse;
}
};

public clearOptionsListCache = () => {
this.cachedOptionsListRequest.cache = new memoize.Cache();
};
}

export interface OptionsListServiceRequiredServices {
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/controls/public/services/options_list/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,6 @@ export interface ControlsOptionsListService {
request: OptionsListRequest,
abortSignal: AbortSignal
) => Promise<OptionsListResponse>;

clearOptionsListCache: () => void;
}
6 changes: 4 additions & 2 deletions src/plugins/embeddable/public/lib/containers/container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,10 @@ export abstract class Container<
this.updateInput(panels as Partial<TContainerInput>);
}

public reload() {
Object.values(this.children).forEach((child) => child.reload());
public reload(clearCache: boolean = false) {
Object.values(this.children).forEach((child) =>
child instanceof ErrorEmbeddable ? child.reload() : child.reload(clearCache)
);
}

public async addNewEmbeddable<
Expand Down
4 changes: 3 additions & 1 deletion src/plugins/embeddable/public/lib/embeddables/embeddable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,14 +119,16 @@ export abstract class Embeddable<
*
* In case if input data did change and reload is requested input$ and output$ would still emit before `reload` is called
*
* @param clearCache can be passed in case there is cache that needs to cleared before reloading the data
*
* The order would be as follows:
* input$
* output$
* reload()
* ----
* updated$
*/
public abstract reload(): void;
public abstract reload(clearCache?: boolean): void;

/**
* Merges input$ and output$ streams and debounces emit till next macro-task.
Expand Down
6 changes: 4 additions & 2 deletions src/plugins/embeddable/public/lib/embeddables/i_embeddable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,11 @@ export interface IEmbeddable<

/**
* Reload the embeddable so output and rendering is up to date. Especially relevant
* if the embeddable takes relative time as input (e.g. now to now-15)
* if the embeddable takes relative time as input (e.g. now to now-15).
*
* @param clearCache can be passed in case there is a need to clear any cache by the embeddable
*/
reload(): void;
reload(clearCache?: boolean): void;

/**
* An embeddable can return inspector adapters if it wants the inspector to be
Expand Down