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

[Logs UI] Refactor query bar state to hooks #52656

Merged
merged 25 commits into from
Jan 3, 2020

Conversation

Zacqary
Copy link
Contributor

@Zacqary Zacqary commented Dec 10, 2019

Summary

Fixes #50397

Moves the Log Filter state handling out of Redux and into a hook.

This is a fairly straightforward refactor.

Checklist

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

For maintainers

@Zacqary Zacqary added v8.0.0 Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.6.0 labels Dec 10, 2019
@Zacqary Zacqary requested a review from a team December 10, 2019 17:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@Zacqary Zacqary added the release_note:skip Skip the PR/issue when compiling release notes label Dec 10, 2019
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

👀 Reviewing on behalf of @elastic/logs-metrics-ui...

Copy link
Contributor

@afgomez afgomez left a comment

Choose a reason for hiding this comment

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

Did a quick shallow pass before I saw @weltenwort's comment. I've only seen minor things, but please wait for his review :)

);

export const LogHighlightsBridge = ({ indexPattern }: { indexPattern: any }) => (
export const LogHighlightsBridge = () => (
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can get rid of this component altogether

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In #50398 we can get rid of it, the log position state still needs the bridge right now though

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

It's nice to see the redux state dwindle 👍

As I commented below I think this can be simplified even more. We'd just have to be careful how and where to access dependencies like the kuery parser.

@Zacqary
Copy link
Contributor Author

Zacqary commented Dec 17, 2019

@elasticmachine merge upstream

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

Nice, this seems to work and looks even better ❤️ I just left a few comments on style and TS idioms below. 😇

@Zacqary
Copy link
Contributor Author

Zacqary commented Dec 20, 2019

Running into some major issues with the useUrlState hook here; it's creating infinite loops of the state updating the url, the url change updating the state, and over and over again. It seems like this hook was written to be a drop-in replacement for useState and doesn't work very well as a bridge.

filterQuery and filterQueryDraft need to sometimes update simultaneously and therefore need to be part of the same useState hook, but only filterQuery is present in the URL. So it's not as simple as refactoring the useLogFilterState hook with useUrlState in place of useState. Might need to take a different approach in order to handle the URL state with hooks.

@Zacqary
Copy link
Contributor Author

Zacqary commented Dec 23, 2019

As per Slack conversation with @weltenwort, we're going to handle the useUrlState hook migration later.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@elastic elastic deleted a comment from elasticmachine Dec 23, 2019
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

LGTM, nice job 👏

@Zacqary
Copy link
Contributor Author

Zacqary commented Jan 3, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@Zacqary Zacqary merged commit a03f395 into elastic:master Jan 3, 2020
Zacqary added a commit that referenced this pull request Jan 3, 2020
* [Logs UI] Refactor query bar state to hooks

* Update typedef

* Typecheck fix

* Typecheck fix

* Simplify log filter state

* Remove WithLogFilter HOC and simplify hook further

* Rename js to ts

* Fix redirect imports

* Fix link-to test accuracy

* Fix link-to test

* Simplify destructuring signature

* Stylistic fixes

* Move URL state to hook

* Fix log filter URL state infinite loop

* Revert "Fix log filter URL state infinite loop"

This reverts commit 43302b3.

* Revert "Move URL state to hook"

This reverts commit c61f5b1.

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 6, 2020
* master:
  increase delay to make sure license refetched (elastic#53882)
  Allow custom NP plugin paths in production (elastic#53562)
  [Maps] show custom color ramps in legend (elastic#53780)
  [Lens] Expression type on document can be null (elastic#53883)
  [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778)
  Update dependency @elastic/charts to v16.0.2 (elastic#52619)
  Set consistent EOL symbol in core API docs (elastic#53815)
  [Logs UI] Refactor query bar state to hooks (elastic#52656)
  [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937)
  Invalidate alert API Key when generating a new one (elastic#53732)
  [Logs UI] HTTP API for log entries (elastic#53798)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 6, 2020
* master:
  increase delay to make sure license refetched (elastic#53882)
  Allow custom NP plugin paths in production (elastic#53562)
  [Maps] show custom color ramps in legend (elastic#53780)
  [Lens] Expression type on document can be null (elastic#53883)
  [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778)
  Update dependency @elastic/charts to v16.0.2 (elastic#52619)
  Set consistent EOL symbol in core API docs (elastic#53815)
  [Logs UI] Refactor query bar state to hooks (elastic#52656)
  [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937)
  Invalidate alert API Key when generating a new one (elastic#53732)
  [Logs UI] HTTP API for log entries (elastic#53798)
  [kbn/pm] add caching to bootstrap (elastic#53622)
  adds createdAt and updatedAt fields to alerting (elastic#53793)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 6, 2020
* master:
  increase delay to make sure license refetched (elastic#53882)
  Allow custom NP plugin paths in production (elastic#53562)
  [Maps] show custom color ramps in legend (elastic#53780)
  [Lens] Expression type on document can be null (elastic#53883)
  [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778)
  Update dependency @elastic/charts to v16.0.2 (elastic#52619)
  Set consistent EOL symbol in core API docs (elastic#53815)
  [Logs UI] Refactor query bar state to hooks (elastic#52656)
  [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937)
  Invalidate alert API Key when generating a new one (elastic#53732)
  [Logs UI] HTTP API for log entries (elastic#53798)
  [kbn/pm] add caching to bootstrap (elastic#53622)
  adds createdAt and updatedAt fields to alerting (elastic#53793)
  [SR] Enable component integration tests (elastic#53893)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 6, 2020
…nsole-dependencies

* 'master' of github.com:elastic/kibana: (33 commits)
  adds strict types to Alerting Client (elastic#53821)
  [Dashboard] Empty screen redesign (elastic#53681)
  Migrate config deprecations and `ShieldUser` functionality to the New Platform (elastic#53768)
  increase delay to make sure license refetched (elastic#53882)
  Allow custom NP plugin paths in production (elastic#53562)
  [Maps] show custom color ramps in legend (elastic#53780)
  [Lens] Expression type on document can be null (elastic#53883)
  [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778)
  Update dependency @elastic/charts to v16.0.2 (elastic#52619)
  Set consistent EOL symbol in core API docs (elastic#53815)
  [Logs UI] Refactor query bar state to hooks (elastic#52656)
  [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937)
  Invalidate alert API Key when generating a new one (elastic#53732)
  [Logs UI] HTTP API for log entries (elastic#53798)
  [kbn/pm] add caching to bootstrap (elastic#53622)
  adds createdAt and updatedAt fields to alerting (elastic#53793)
  [SR] Enable component integration tests (elastic#53893)
  Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794)
  moved Task Manager server code under "server" directory (elastic#53777)
  Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886)
  ...

# Conflicts:
#	yarn.lock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Logs UI Logs UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logs UI] Refactor query bar state to hooks
5 participants