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

Changes the way Events are handled in the Visualization Library #1773

Merged
merged 22 commits into from
Nov 3, 2014

Conversation

stormpython
Copy link
Contributor

This pull request addresses issues with the way events are currently handled in the visualization library.

Currently, in order to listen for events on the chart, the addEvents flag needs to be set to true in the params passed to the chart upon initialization. Given that this is not an ideal way to handle events, a refactoring of the code was in order.

With this pull request, events can now be added to the Vis object, and they will be bound to each chart. There is no need to set an addEvents flag. For example:

// initialize chart with DOM element and vislib params
var chart = new Vis($el, vislibParams);

// add events and listeners to the chart(s)
chart.on('click', function (e) {
  console.log(e);
});

This is all you will need to do in order to add events to charts. You will also be able to add multiple listeners for each event. You can also add events to charts before the render method is called as well as after. For example:

// add events before the render method
chart.on('click', function (e) {
  console.log(e);
});

// render chart
chart.render(data);

// add events with listener after chart(s) are rendered
chart.on('brush', function (e) {
  console.log('brushing');
});

This will cause both 'click' and 'brush' events to be bound to the chart(s).

To turn off events, you have two options.

  1. Remove a listener from an event array
  2. Remove the event all together

For example:

// 1. Remove a listener from an event array
chart.off('click', function (e) {
  console.log(e);
});

// 2. Remove the event all together
chart.off('click');

In example 1, if the click array has several listeners (i.e. the array !== 0), then only the specified listener is removed. The chart still listens for click events and emits the event to the listeners in the array. However, if the array.length === 0, then the event is removed from the chart.

In example 2, the 'click' event is removed all together from the chart. Events can be removed both before and after charts have been rendered.

Changes were made in several places in the code base to accommodate the changes to the way events are handled. Changes to the parts described above are found in the vis.js and the handler.js files.

In order to reduce duplicate code in the visualization modules, the visualization event code was consolidated into the dispatch.js class. This move required a change in the way we were handling styling changes with events, as each chart required slightly different styling for events. The css styling was moved out of javascript and into the less files. And since the less files were a mess and disorganized, they were refactored and broken up into smaller, more manageable files.

Finally, tests were written for the on and off methods in vis.js and for the enable and disable methods in handler.js. Tests still need to be written for dispatch.js methods as well as all visualization modules.

@rashidkpc
Copy link
Contributor

Not currently mergable, but it looks like this is going to break existing interactions? Can you update any existing usages and get with @simianhacker to make sure he's happy with the new implementation?

@stormpython
Copy link
Contributor Author

@rashidkpc I am not sure what you mean by break existing interactions. If you are talking about interactions with the current charts, the needed changes should be in this pull request. Unless you've tested it out and found some bugs that I am unaware of.

I also had a sit down earlier with @simianhacker to go over the changes. But happy to have another one if he wants.

@stormpython
Copy link
Contributor Author

Tests are passing for me locally. Looks like a build failure.

rashidkpc added a commit that referenced this pull request Nov 3, 2014
Changes the way Events are handled in the Visualization Library
@rashidkpc rashidkpc merged commit f7ddf50 into elastic:master Nov 3, 2014
@stormpython stormpython deleted the feature/fix_events branch November 4, 2014 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants