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

[Canvas] Fixes filter clearing on undo/redo #31859

Merged
merged 4 commits into from
Feb 25, 2019

Conversation

cqliu1
Copy link
Contributor

@cqliu1 cqliu1 commented Feb 22, 2019

Summary

Closes #28730.

This adds a check in the setExpression action to clear out the filters if the expression is no longer a filter expression. This makes sure filters don't persist if you started with a filter element and changed the expression to another type of element, such as markdown.

feb-22-2019 16-31-20

This also removes the call to clear element filters from the filter element onDestroy handler which was clearing out the element's filter when you undo/redo or nav between workpad pages.

feb-22-2019 16-32-49

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] 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
- [ ] Unit or functional tests were updated or added to match the most common scenarios
- [ ] This was checked for keyboard-only and screenreader accessibility

For maintainers

@cqliu1 cqliu1 added review v7.0.0 Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.0.0 v6.7.0 v7.2.0 labels Feb 22, 2019
@cqliu1 cqliu1 requested a review from w33ble February 22, 2019 23:12
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

@cqliu1 cqliu1 requested a review from a team as a code owner February 22, 2019 23:12
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

// reset element.filter if element is no longer a filter
if (
updatedElement.filter &&
!['dropdownControl', 'timefilterControl', 'exactly'].some(filter =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we can get the list of filter elements without hard-coding them here? Maybe we can pull the functions from the registry somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd need to somehow check for render functions that specifically render filters. I don't see anything in the renderer def that I could use to single out filter renderers.

I could introduce a new property in the renderer def like subtype: filter, but I think this should be addressed in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I gave this some thought too, and couldn't come up with anything simple here. I'm cool derfering on this as well, just please open an issue for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a TODO to that line, and I'll open an issue.

@w33ble
Copy link
Contributor

w33ble commented Feb 25, 2019

This is a ton better, but there's still an edge case here that involves the advanced filter. It may be a bug in that component though, and I'm fine with deferring and opening another issue instead.

Here's the edge case:

  • load the flights workpad (reset sample data if needed)
  • delete the existing time filter (this makes it easier to watch the values in state)
  • add new time filter (now you can just watch the last element in the list)
  • change field to "timestamp"

screenshot 2019-02-25 10 41 36

So far so good, we see the filter value we expect to see. Now:

  • change expression to timefilterControl compact=true column="timestamp" | render as=advanced_filter
  • look at the filter value, it shows the wrong value

screenshot 2019-02-25 10 42 46

Note that column is still set to the original @timestamp value, not the updated timestamp value.

If you change the filter value and click apply, and then remove the render as=advanced_filter part, the time filter does show the updated value, which is good, but it seems to get out of sync the other way.

As stated, I'm fine deferring this, advanced_filter is kind of a hidden element, it's not really documented and doesn't show up in auto-complete, so users are probably unlikely to run into this.

Copy link
Contributor

@w33ble w33ble left a comment

Choose a reason for hiding this comment

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

LGTM as-is, but see my comment about the edge case. If you don't want to fix it here (which is probably the right call), please open a new issue.

@cqliu1 cqliu1 changed the title Fixes filter clearing on undo/redo [Canvas] Fixes filter clearing on undo/redo Feb 25, 2019
@cqliu1
Copy link
Contributor Author

cqliu1 commented Feb 25, 2019

@w33ble I opened #31952 for the advanced_filter issue.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cqliu1 cqliu1 merged commit a037874 into elastic:master Feb 25, 2019
@cqliu1 cqliu1 deleted the fix/reset-filters branch February 25, 2019 23:26
cqliu1 added a commit to cqliu1/kibana that referenced this pull request Feb 25, 2019
* Added check to reset filters in 'setExpression'

* Removed filter reset from filter renderers' onDestroy handler

* Cleaned up setExpression

* Added TODO
cqliu1 added a commit to cqliu1/kibana that referenced this pull request Feb 25, 2019
* Added check to reset filters in 'setExpression'

* Removed filter reset from filter renderers' onDestroy handler

* Cleaned up setExpression

* Added TODO
cqliu1 added a commit to cqliu1/kibana that referenced this pull request Feb 25, 2019
* Added check to reset filters in 'setExpression'

* Removed filter reset from filter renderers' onDestroy handler

* Cleaned up setExpression

* Added TODO
cqliu1 added a commit that referenced this pull request Feb 26, 2019
* Added check to reset filters in 'setExpression'

* Removed filter reset from filter renderers' onDestroy handler

* Cleaned up setExpression

* Added TODO
cqliu1 added a commit that referenced this pull request Feb 26, 2019
* Added check to reset filters in 'setExpression'

* Removed filter reset from filter renderers' onDestroy handler

* Cleaned up setExpression

* Added TODO
cqliu1 added a commit that referenced this pull request Feb 26, 2019
* Added check to reset filters in 'setExpression'

* Removed filter reset from filter renderers' onDestroy handler

* Cleaned up setExpression

* Added TODO
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:fix review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v6.7.0 v7.0.0 v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Canvas] Undo/redo is setting and clearing filters
3 participants