Skip to content

Fixes filter on values that are not in the result #20608

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 12, 2018
Merged

Fixes filter on values that are not in the result #20608

merged 2 commits into from
Jul 12, 2018

Conversation

@ppisljar
Copy link
Contributor

@ppisljar ppisljar commented Jul 10, 2018 •
edited
Loading

fixes filtering on countries with no data in region map

resolves #20407

@ppisljar ppisljar added review Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v6.4.0 labels Jul 10, 2018
@ppisljar ppisljar requested review from markov00 and timroes July 10, 2018 07:35
@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 10, 2018

💔 Build Failed

@ppisljar
Copy link
Contributor Author

ppisljar commented Jul 10, 2018 

07:36:30 fatal: Could not read from remote repository.
07:36:30 
07:36:30 Please make sure you have the correct access rights

jenkins, test this

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 10, 2018

💔 Build Failed

timroes
timroes reviewed Jul 10, 2018
View reviewed changes
src/ui/public/vis/vis.js Outdated
Copy link
Contributor

@timroes timroes Jul 10, 2018

Choose a reason for hiding this comment

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

I am not a huge fan of that syntax, that we need to pass in a rowIndex even in cases we might not have it, but I see, that addFilter(data, { columnIndex, rowIndex, cellValue }) might also not be the best solution (especially since we know we need the columnIndex and addFilter(data, columnIndex, { rowIndex, cellValue }) for sure looks even more weird...

So just wanted to point that out, even without having a proper good solution (but maybe @markov00 or so has an idea).

Could we at least give cellValue = null a default value?

Copy link
Member

@markov00 markov00 Jul 10, 2018

Choose a reason for hiding this comment

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

As written few seconds ago on my review, are we sure that this is the right behaviour? if we don't have data, why we want to filter for a region with no data?
What if we avoid the click and mouseover on regions with no data at all?

Copy link
Contributor Author

@ppisljar ppisljar Jul 10, 2018

Choose a reason for hiding this comment

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

this is the behaviour we had before ... and i think it makes sense ...

using makelogs data:

  • create region map, select term as geo.src, leave size at 5 ....
  • you see the top 5 countries ... however you can now click on something like germany for example (which does not have color applied at the moment as its not top 5) and a filter will apply and you will see the color on germany.

@timroes
Copy link
Contributor

timroes commented Jul 10, 2018

We should add a functional test, that covers that failure.

markov00
markov00 reviewed Jul 10, 2018
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Tested on chrome/osx. Everything works.
Do we want to check with @elastic/kibana-gis team if this is the wanted behaviour when we click on a region with no data?

@ppisljar
Copy link
Contributor Author

ppisljar commented Jul 10, 2018

jenkins, test this

@timroes timroes changed the title fixes region map filters Fixes filter on values that are not in the result Jul 10, 2018
@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 10, 2018

💔 Build Failed

@ppisljar
Copy link
Contributor Author

ppisljar commented Jul 11, 2018

jenkins, test this

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 11, 2018

💔 Build Failed

@ppisljar ppisljar force-pushed the fix/regionMapFilter branch from a0684ba to 1a93bc1 Compare July 12, 2018 07:16
@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 12, 2018

💚 Build Succeeded

timroes
timroes approved these changes Jul 12, 2018
View reviewed changes
Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Code looks good to me, tested that region map issue in Chrome, Linux is fixed.

@ppisljar ppisljar merged commit 1726665 into elastic:master Jul 12, 2018
ppisljar added a commit to ppisljar/kibana that referenced this pull request Jul 12, 2018
@ppisljar
Fixes filter on values that are not in the result (elastic#20608)
20e1fad
@ppisljar ppisljar mentioned this pull request Jul 12, 2018
Merged
@ppisljar ppisljar deleted the fix/regionMapFilter branch July 12, 2018 11:37
ppisljar added a commit that referenced this pull request Jul 12, 2018
@ppisljar
Fixes filter on values that are not in the result (#20608) (#20713)
0d24339
@ppisljar ppisljar restored the fix/regionMapFilter branch September 26, 2018 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markov00 markov00 markov00 approved these changes

+1 more reviewer

@timroes timroes timroes approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v6.4.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug clicking in region map on empty regions

4 participants

@ppisljar @elasticmachine @timroes @markov00