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

Fix filtering from visualizations #7793

Merged
merged 1 commit into from
Jul 22, 2016
Merged

Fix filtering from visualizations #7793

merged 1 commit into from
Jul 22, 2016

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Jul 21, 2016

The ability to create filters by clicking on visualizations was broken
between 5.0 alpha3 and alpha4. The root cause was the addition of a new
parameter being sent to all visualization event handlers, which was
added to support persistence of tilemap position and zoom level. The
filter_bar_click_handler (which enables the vis filtering behavior)
already expected a different kind of second parameter, so the addition
of this new parameter threw off the click handler's internal logic.

It turns out that the new parameter is unnecessary, because the object
it's passing is already available on the event object. So to fix this,
I removed the new parameter from the emitter and updated the tilemap
handlers to grab the object off of the event.

Fixes: #7501
Related: #6835 (added the new param)

@Bargs
Copy link
Contributor Author

Bargs commented Jul 21, 2016

@tbragin perhaps you'd like to test this one as well?

@jbudz
Copy link
Member

jbudz commented Jul 21, 2016

Awesome, testing now and everything is working. can you point to where the third argument was breaking things, it's not immediately clear to me.

@jbudz
Copy link
Member

jbudz commented Jul 21, 2016

Is it worth adding a test for not passing three arguments?

@Bargs
Copy link
Contributor Author

Bargs commented Jul 21, 2016

@jbudz

When simulate is truthy, the filters don't get added to the app state. I didn't look into where simulate actually gets used, I assume it's for testing.

@Bargs
Copy link
Contributor Author

Bargs commented Jul 21, 2016

I think a test is worthwhile, lemme try to whip something up.

The ability to create filters by clicking on visualizations was broken
between 5.0 alpha3 and alpha4. The root cause was the addition of a new
parameter being sent to all visualization event handlers, which was
added to support persistence of tilemap position and zoom level. The
filter_bar_click_handler (which enables the vis filtering behavior)
already expected a different kind of second parameter, so the addition
of this new parameter threw off the click handler's internal logic.

It turns out that the new parameter is unnecessary, because the object
it's passing is already available on the event object. So to fix this,
I removed the new parameter from the emitter and updated the tilemap
handlers to grab the object off of the event.

Fixes: elastic#7501
Related: elastic#6835 (added the new
param)
@Bargs
Copy link
Contributor Author

Bargs commented Jul 21, 2016

@jbudz pushed a test

@jbudz
Copy link
Member

jbudz commented Jul 21, 2016

LGTM

@Bargs
Copy link
Contributor Author

Bargs commented Jul 21, 2016

@ycombinator since you're familiar with the PR that introduced the bug, would you mind giving this a second set of eyes?

@Bargs Bargs assigned ycombinator and unassigned jbudz Jul 21, 2016
@tbragin
Copy link
Contributor

tbragin commented Jul 21, 2016

@Bargs I did a quick test and works for me functionally. Thank you so much for your help on this!

@ycombinator
Copy link
Contributor

Functionality LGTM. Reviewing code now...

@ycombinator
Copy link
Contributor

The root cause was the addition of a new parameter being sent to all visualization event handlers, which was added to support persistence of tilemap position and zoom level. The filter_bar_click_handler (which enables the vis filtering behavior) already expected a different kind of second parameter, so the addition of this new parameter threw off the click handler's internal logic.

Thanks for this clear explanation. Can we have type safety now? 👅

@ycombinator
Copy link
Contributor

LGTM. 👍 for adding that strict unit test.

@ycombinator ycombinator assigned Bargs and unassigned ycombinator Jul 22, 2016
@Bargs Bargs merged commit 287cf99 into elastic:master Jul 22, 2016
@epixa epixa added v5.0.0 and removed v5.0.0 labels Aug 1, 2016
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
Fix filtering from visualizations

Former-commit-id: 287cf99
cee-chen added a commit that referenced this pull request Jun 18, 2024
`v94.6.0` ⏩ `v95.0.0-backport.0`

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

##
[`v95.0.0-backport.0`](https://github.com/elastic/eui/releases/v95.0.0-backport.0)

**This is a backport release only intended for use by Kibana.**

- Updated `EuiSteps` to support a new `titleSize="xxs"` style, which
outputs the same title font size but smaller unnumbered step indicators
([#7813](elastic/eui#7813))
- Updated `EuiStepsHorizontal` to support a new `size="xs"` style, which
outputs smaller unnumbered step indicators
([#7813](elastic/eui#7813))
- Updated `EuiStepNumber` to support new `titleSize="none"` which omits
rendering step numbers, and will only render icons
([#7813](elastic/eui#7813))

## [`v95.0.0`](https://github.com/elastic/eui/releases/v95.0.0)

- Added `move` glyph to `EuiIcon`
([#7789](elastic/eui#7789))
- Updated `EuiBasicTable` and `EuiInMemoryTable`s with `selection` - the
header row checkbox will now render an indeterminate state if some (but
not all) rows are selected
([#7817](elastic/eui#7817))

**Bug fixes**

- Fixed an `EuiDataGrid` visual bug when using `lineCount` row heights
where the clamped text was still visible for some font sizes
([#7793](elastic/eui#7793))
- Fixed `EuiSearchBar`'s filter configs to always respect `autoClose:
false` ([#7806](elastic/eui#7806))

**Breaking changes**

- Removed deprecated `EUI_CHARTS_THEME_DARK`, `EUI_CHARTS_THEME_LIGHT`
and `EUI_SPARKLINE_THEME_PARTIAL` exports
([#7682](elastic/eui#7682))
- Removed deprecated `euiPalettePositive` and `euiPaletteNegative`. Use
`euiPaletteGreen` and `euiPaletteRed` instead
([#7808](elastic/eui#7808))
- Removed `type="inList"` from `EuiCheckbox`. Simply omit passing a
`label` prop to render this style of checkbox
([#7814](elastic/eui#7814))
- Removed the unused `compressed` prop from `EuiCheckbox` and
`EuiRadio`. This prop was not doing anything on individual components.
([#7818](elastic/eui#7818))

**CSS-in-JS conversions**

- Converted `EuiCheckboxGroup` to Emotion
([#7818](elastic/eui#7818))
- Converted `EuiRadioGroup` to Emotion
([#7818](elastic/eui#7818))

---------

Co-authored-by: Cee Chen <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Cee Chen <[email protected]>
bhapas pushed a commit to bhapas/kibana that referenced this pull request Jun 18, 2024
`v94.6.0` ⏩ `v95.0.0-backport.0`

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

##
[`v95.0.0-backport.0`](https://github.com/elastic/eui/releases/v95.0.0-backport.0)

**This is a backport release only intended for use by Kibana.**

- Updated `EuiSteps` to support a new `titleSize="xxs"` style, which
outputs the same title font size but smaller unnumbered step indicators
([elastic#7813](elastic/eui#7813))
- Updated `EuiStepsHorizontal` to support a new `size="xs"` style, which
outputs smaller unnumbered step indicators
([elastic#7813](elastic/eui#7813))
- Updated `EuiStepNumber` to support new `titleSize="none"` which omits
rendering step numbers, and will only render icons
([elastic#7813](elastic/eui#7813))

## [`v95.0.0`](https://github.com/elastic/eui/releases/v95.0.0)

- Added `move` glyph to `EuiIcon`
([elastic#7789](elastic/eui#7789))
- Updated `EuiBasicTable` and `EuiInMemoryTable`s with `selection` - the
header row checkbox will now render an indeterminate state if some (but
not all) rows are selected
([elastic#7817](elastic/eui#7817))

**Bug fixes**

- Fixed an `EuiDataGrid` visual bug when using `lineCount` row heights
where the clamped text was still visible for some font sizes
([elastic#7793](elastic/eui#7793))
- Fixed `EuiSearchBar`'s filter configs to always respect `autoClose:
false` ([elastic#7806](elastic/eui#7806))

**Breaking changes**

- Removed deprecated `EUI_CHARTS_THEME_DARK`, `EUI_CHARTS_THEME_LIGHT`
and `EUI_SPARKLINE_THEME_PARTIAL` exports
([elastic#7682](elastic/eui#7682))
- Removed deprecated `euiPalettePositive` and `euiPaletteNegative`. Use
`euiPaletteGreen` and `euiPaletteRed` instead
([elastic#7808](elastic/eui#7808))
- Removed `type="inList"` from `EuiCheckbox`. Simply omit passing a
`label` prop to render this style of checkbox
([elastic#7814](elastic/eui#7814))
- Removed the unused `compressed` prop from `EuiCheckbox` and
`EuiRadio`. This prop was not doing anything on individual components.
([elastic#7818](elastic/eui#7818))

**CSS-in-JS conversions**

- Converted `EuiCheckboxGroup` to Emotion
([elastic#7818](elastic/eui#7818))
- Converted `EuiRadioGroup` to Emotion
([elastic#7818](elastic/eui#7818))

---------

Co-authored-by: Cee Chen <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Cee Chen <[email protected]>
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.

Can't add filter from visualizations
5 participants