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

Expose filter item from data plugin #94546

Merged
merged 3 commits into from
Mar 18, 2021

Conversation

shahzad31
Copy link
Contributor

Summary

we are using this component in our exploratory view #94426

so exposing it from data plugin.

@shahzad31 shahzad31 marked this pull request as ready for review March 15, 2021 11:56
@shahzad31 shahzad31 requested a review from a team as a code owner March 15, 2021 11:56
@shahzad31 shahzad31 self-assigned this Mar 15, 2021
@shahzad31 shahzad31 added release_note:skip Skip the PR/issue when compiling release notes v7.13.0 v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Mar 15, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

LGTM, didn't test, one suggest 👇

@@ -154,83 +157,90 @@ export function FilterItem(props: Props) {

function getPanels() {
const { negate, disabled } = filter.meta;
return [
let mainPanelItems = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we type this EuiContextMenuPanelItemDescriptor & {id: PanelOptions} and then use id for filtering hiddenPanelOptions 🙏

Would be nice to have hiddenPanelOptions type connected to actually panel items and not use an arbitrary data-test-subj string for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i tried adding id or using EuiContextMenuPanelItemDescriptor, typescript goes crazy in both cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

working with EUI items types can be tricky , so i am not sure, how to solve this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, indeed tricky!

To workaround eui types, I ended up with:

type PanelOptions = 'pinFilter' | 'editFilter' | 'negateFilter' | 'disableFilter' | 'deleteFilter';

interface PanelItem {
  id: PanelOptions;
  euiPanel: EuiContextMenuPanelItemDescriptor;
}

....

let mainPanelItems: PanelItem[] = [
      {
        id: 'pinFilter',
        euiPanel: {
          name: isFilterPinned(filter)
            ? props.intl.formatMessage({
                id: 'data.filter.filterBar.unpinFilterButtonLabel',
                defaultMessage: 'Unpin',
              })
            : props.intl.formatMessage({
                id: 'data.filter.filterBar.pinFilterButtonLabel',
                defaultMessage: 'Pin across all apps',
              }),
          icon: 'pin',
          onClick: () => {
            setIsPopoverOpen(false);
            onTogglePinned();
          },
          'data-test-subj': 'pinFilter',
        },
      },
....
    ];

And then filter panels by id: 'pinFilter',

to be honest, I still think it is worth it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^^ i don't think that will work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so i am leaving this improvement perhaps for future :)

@kibanamachine
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
data 211.3KB 214.2KB +2.9KB

Page load bundle

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

id before after diff
data 807.6KB 807.4KB -177.0B
Unknown metric groups

async chunk count

id before after diff
data 9 11 +2

History

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

cc @shahzad31

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Change LGTM,

I still think it is worth improving type binding within the component:
https://github.com/elastic/kibana/pull/94546/files#r596662205

@shahzad31 shahzad31 merged commit 95b7635 into elastic:master Mar 18, 2021
@shahzad31 shahzad31 deleted the expose-filter-item-component branch March 18, 2021 18:11
@shahzad31 shahzad31 added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 18, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

7.x / #94964

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Mar 18, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kibanamachine added a commit that referenced this pull request Mar 18, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Shahzad <shahzad.muhammad@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants