fixing region map click filter#61949
Conversation
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
wylieconlon
left a comment
There was a problem hiding this comment.
The functionality is working for me, tested in the visualize editor and dashboards. I don't understand why this broke, and it makes me worry that there are under-tested areas of this code. Can a unit test trigger this select event, as well as any other events that are happening on the map? I think a unit test would be called for here.
|
i don't think a unit test can help, we need integration testing,
however when maps was emitting wrong information, due to the embeddable changes and small change in data format, when emitting an action action would not fire (as rowIndex at this point was -1) i think unit tests would very unlikely catch this |
|
@elasticmachine merge upstream |
|
@ppisljar Can you be more specific about why this broke? You wrote that it was "due to the embeddable changes and small change in data format," can you link to specific PRs? My concern is that we didn't catch this in an automated way at all, and that other code could be broken too. |
|
#55351 in this PR we removed cellValue property from createFiltersFromEvent function. regionmap was always passing rowIndex as -1 but depending on the behaviour of passing in correct value. |
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
wylieconlon
left a comment
There was a problem hiding this comment.
I am trying to review this carefully because I am not familiar with how filters are applied in these visualizations. It seems like I need to trace out how this code used to work, vs how it works now.
wylieconlon
left a comment
There was a problem hiding this comment.
I've now understood the behavior of this change, it definitely works now.
| } | ||
|
|
||
| this._choroplethLayer.on('select', event => { | ||
| const rowIndex = this._chartData.rows.findIndex(row => row[0] === event); |
There was a problem hiding this comment.
Okay, so it looks like rowIndex was always returning -1 before, and there used to be a special case for this.
Summary
resolves #61879
Checklist
Delete any items that are not applicable to this PR.
[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] This was checked for keyboard-only and screenreader accessibility[ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser[ ] This was checked for cross-browser compatibility, including a check against IE11For maintainers
[ ] This was checked for breaking API changes and was labeled appropriatelyfunctional test should be added but i am not doing it for now as its not going to be easy (figuring out where on
<canvas>do we need to click)