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

Disable clicks on charts #41768

Closed
wants to merge 3 commits into from

Conversation

FilipRy
Copy link
Contributor

@FilipRy FilipRy commented Jul 23, 2019

Summary

There are use cases when you don't wish to create filter on data, visualized in dashboard/visualizations's charts after you click on a certain part of the chart. Therefore, I added a feature enabling the users to disable clicking on charts.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@legrego legrego added review Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jul 23, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@timroes
Copy link
Contributor

timroes commented Jul 29, 2019

Hi,

thanks a lot for your contribution. Given the actual content I fear we need some discussion around other refactoring currently going on.

@stacey-gammon @ppisljar I am wondering if the Event/Actions refactoring will make this PR redundant? Will that new architecture allow us to disable that kind of events for any chart without needing an individual implementation for every chart?

@timroes timroes self-assigned this Jul 29, 2019
@stacey-gammon
Copy link
Contributor

Will that new architecture allow us to disable that kind of events for any chart without needing an individual implementation for every chart?

Until right now I was assuming each chart would implement because they render and register the clicks, but now thinking about it more, it'd probably be easier and more generic to implement this at the embeddable level and render an invisible click catching div over everything to prevent clicks from ever making it to the content below.

@timroes
Copy link
Contributor

timroes commented Jul 29, 2019

Looking at the code here, this one neither does "prevent clicks" it will just no longer emit an event for it. Wouldn't that be the same, then just removing all click actions on the chart? The event would still be emitted in that case, but neither have any effect (same as in this PR)?

@stacey-gammon
Copy link
Contributor

Wouldn't that be the same, then just removing all click actions on the chart? The event would still be emitted in that case, but neither have any effect (same as in this PR)?

Ah, yea actually I think that would probably work too, as long as all the click actions were using actions. You are thinking something like this?: (using search embeddable as an example since visualizations bypass the action API)

instead of:

    searchScope.filter = async (field, value, operator) => {
      const index = this.savedSearch.searchSource.getField('index').id;

      let filters = this.filterGen.generate(field, value, operator, index);
      filters = filters.map(filter => ({
        ...filter,
        $state: { store: FilterStateStore.APP_STATE },
      }));

      await executeTriggerActions(APPLY_FILTER_TRIGGER, {
        embeddable: this,
        triggerContext: {
          filters,
        },
      });
    };

    this.searchScope = searchScope;
  }

do something like this:

    searchScope.filter = async (field, value, operator) => {
     ** if (this.input.interactionDisabled) return;**

      const index = this.savedSearch.searchSource.getField('index').id;

      let filters = this.filterGen.generate(field, value, operator, index);
      filters = filters.map(filter => ({
        ...filter,
        $state: { store: FilterStateStore.APP_STATE },
      }));

      await executeTriggerActions(APPLY_FILTER_TRIGGER, {
        embeddable: this,
        triggerContext: {
          filters,
        },
      });
    };

    this.searchScope = searchScope;
  }

@timroes
Copy link
Contributor

timroes commented Jul 29, 2019

@stacey-gammon for phase 1: yes. I was actually already thinking phase 2 again, and thought we simply give the user a toggle on each panel: "Disable all interaction", that will does that for them?

@stacey-gammon
Copy link
Contributor

@timroes - I don't think we need phase 2 for this. disableInteraction is just an input variable that can be set from a panel action, even in phase 1. We might also want to have it at the top level too, like "hide all panel titles" is a container level setting. So,

we simply give the user a toggle on each panel: "Disable all interaction", that will does that for them?

yes that would work.

But first we need to make sure visualizations use the action API and bubble filter events up rather than using the filter service directly. I don't think we have an issue for this yet but it's mentioned here: #32371. Not sure how it works on maps.

@ppisljar
Copy link
Member

i agree this should be implemented on embeddable level as we'll want this functionality for maps, lens etc as well.

@timroes
Copy link
Contributor

timroes commented Jul 30, 2019

Hi @FilipRy,

thanks again for your contribution. Unfortunately due to some larger Embeddable/Action refactoring we're currently doing, we don't want to move forward with adding this feature individually to each charts. I am sorry that we'll close that PR for now, but once we're further with that refactoring I hope we can provide that solution in a universal way (so it doesn't need to be implemented by every chart individually).

I am sorry that we don't move forward with that now, and hope that won't block you from further contributions to the ecosystem.

Thank you a lot,
Tim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants