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

[ML] Deprecates the use of jQuery for rendering Anomaly Explorer Swimlanes #23000

Merged
merged 7 commits into from
Sep 14, 2018

Conversation

walterra
Copy link
Contributor

This refactor gets rid of jQuery as a requirement for the Anomaly Explorer Swimlanes. All the DOM manipulation previously done by jQuery is now done by d3.

Notes:

  • The primary aim here is to replace jQuery with d3. To avoid refactoring regressions, this PR sticks as much as possible to the original structure. Overall, this could be done in a more "d3"-way, but it's a good start.
  • The resulting DOM is a bit different: The structure is exactly the same, just how we use certain data-attributes changed.
  • Instead of using jQuery's data-feature for storing click event data, that data is now directly stored on DOM nodes. Again, this could be done in a more "d3"-way but would require quite some further refactoring.

Possible Todos for a follow-up:

  • Move away from the approach of storing data directly on DOM nodes.
  • Event callbacks like clearSelection() or selectCell() shouldn't really manipulate the DOM. They should just manipulate state and trigger a re-render. The rendering function then should take care of considering that state change and render accordingly.

Part of #22642.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

@elasticmachine
Copy link
Contributor

💔 Build Failed

@walterra
Copy link
Contributor Author

retest

$('.sl-cell-inner,.sl-cell-inner-dragselect', '.ml-explorer-swimlane').addClass('sl-cell-inner-masked');
$('.sl-cell-inner.sl-cell-inner-selected,.sl-cell-inner-dragselect.sl-cell-inner-selected',
'.ml-explorer-swimlane').removeClass('sl-cell-inner-selected');
// This select both overall and viewby swimlane
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - should be This selects ...

$('.sl-cell-inner-dragselect.sl-cell-inner-selected', '.ml-explorer-swimlane').removeClass('sl-cell-inner-selected');
$('.ds-selected', '.ml-explorer-swimlane').removeClass('ds-selected');

// This select both overall and viewby swimlane
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - should be selects

return (e) => {
cellMouseover(e, safeLaneTxt, bucketScore, i, time);
};
const safeLaneTxt = lane.replace(/(['\\])/g, '\\$1');
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested this with any datasets which contain partitioning field names and values with 'special' characters in, such as quotes, spaces, backslashes. Just worth double checking that all the functionality works as expected.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@walterra walterra merged commit 4663991 into elastic:master Sep 14, 2018
@walterra walterra deleted the ml-d3-swimlane branch September 14, 2018 16:20
walterra added a commit to walterra/kibana that referenced this pull request Sep 14, 2018
…lanes (elastic#23000)

- This refactor gets rid of jQuery as a requirement for the Anomaly Explorer Swimlanes. All the DOM manipulation previously done by jQuery is now done by d3.
- The primary aim here is to replace jQuery with d3. To avoid refactoring regressions, this PR sticks as much as possible to the original structure. Overall, this could be done in a more "d3"-way, but it's a good start.
- The resulting DOM is a bit different: The structure is exactly the same, just how we use certain data-attributes changed.
- Instead of using jQuery's data-feature for storing click event data, that data is now directly stored on DOM nodes. Again, this could be done in a more "d3"-way but would require quite some further refactoring.
walterra added a commit that referenced this pull request Sep 17, 2018
…lanes (#23000) (#23193)

- This refactor gets rid of jQuery as a requirement for the Anomaly Explorer Swimlanes. All the DOM manipulation previously done by jQuery is now done by d3.
- The primary aim here is to replace jQuery with d3. To avoid refactoring regressions, this PR sticks as much as possible to the original structure. Overall, this could be done in a more "d3"-way, but it's a good start.
- The resulting DOM is a bit different: The structure is exactly the same, just how we use certain data-attributes changed.
- Instead of using jQuery's data-feature for storing click event data, that data is now directly stored on DOM nodes. Again, this could be done in a more "d3"-way but would require quite some further refactoring.
@sophiec20 sophiec20 added the Feature:Anomaly Detection ML anomaly detection label Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants