[Logs UI] Refactor query bar state to hooks by Zacqary · Pull Request #52656 · elastic/kibana

Zacqary · 2019-12-10T17:19:25Z

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.

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

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-12-10T17:19:28Z

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

weltenwort

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

afgomez

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

afgomez · 2019-12-11T09:23:11Z

x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/redux_bridges.tsx

-);
-
-export const LogHighlightsBridge = ({ indexPattern }: { indexPattern: any }) => (
+export const LogHighlightsBridge = () => (


I guess we can get rid of this component altogether

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

x-pack/legacy/plugins/infra/public/containers/logs/log_filter/index.ts

weltenwort

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.

x-pack/legacy/plugins/infra/public/containers/logs/log_filter/index.ts

x-pack/legacy/plugins/infra/public/containers/logs/with_log_filter.tsx

…g-filters # Conflicts: # x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/redux_bridges.tsx

…g-filters

Zacqary · 2019-12-17T17:48:08Z

@elasticmachine merge upstream

…a into 50397-refactor-log-filters

weltenwort

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

x-pack/legacy/plugins/infra/public/containers/logs/log_filter/log_filter_state.ts

x-pack/legacy/plugins/infra/public/containers/logs/log_filter/with_log_filter_url_state.tsx

x-pack/legacy/plugins/infra/public/containers/logs/log_filter/log_filter_state.ts

x-pack/test/functional/apps/infra/link_to.ts

x-pack/legacy/plugins/infra/public/containers/logs/log_filter/log_filter_state.ts

x-pack/legacy/plugins/infra/public/containers/logs/log_filter/with_log_filter_url_state.tsx

x-pack/legacy/plugins/infra/public/containers/logs/log_filter/log_filter_state.ts

…g-filters # Conflicts: # x-pack/test/functional/apps/infra/link_to.ts

…g-filters

Zacqary · 2019-12-20T22:15:57Z

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.

…g-filters

This reverts commit 43302b3.

This reverts commit c61f5b1.

Zacqary · 2019-12-23T17:19:07Z

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

kibanamachine · 2019-12-23T17:28:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9063d77

History

💚 Build #16984 succeeded 43302b3
💔 Build #16957 failed a7fe3ef
💔 Build #16800 failed c61f5b1
💚 Build #16764 succeeded 8cb525d
💚 Build #16745 succeeded ef00491

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

kibanamachine · 2019-12-23T18:35:39Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9a7e15d

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

…g-filters

weltenwort

LGTM, nice job 👏

Zacqary · 2020-01-03T19:33:26Z

@elasticmachine merge upstream

kibanamachine · 2020-01-03T20:53:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ce3a832

History

💚 Build #17277 succeeded 3cbf030

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

* [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 <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* 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)

* 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)

* 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)

…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

Zacqary added 2 commits December 10, 2019 11:14

[Logs UI] Refactor query bar state to hooks

ba2c456

Update typedef

f91debd

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 requested a review from a team December 10, 2019 17:19

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

Zacqary added 2 commits December 10, 2019 15:20

Typecheck fix

4e5035d

Typecheck fix

2759d2f

weltenwort reviewed Dec 11, 2019

View reviewed changes

weltenwort assigned Zacqary Dec 11, 2019

afgomez reviewed Dec 11, 2019

View reviewed changes

weltenwort requested changes Dec 11, 2019

View reviewed changes

x-pack/legacy/plugins/infra/public/containers/logs/log_filter/index.ts Outdated Show resolved Hide resolved

x-pack/legacy/plugins/infra/public/containers/logs/with_log_filter.tsx Outdated Show resolved Hide resolved

Zacqary added 6 commits December 12, 2019 11:51

Simplify log filter state

c16e03c

Merge remote-tracking branch 'upstream/master' into 50397-refactor-lo…

5798608

…g-filters # Conflicts: # x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/redux_bridges.tsx

Remove WithLogFilter HOC and simplify hook further

57b5b18

Rename js to ts

ede66d8

Fix redirect imports

968b5a7

Merge remote-tracking branch 'upstream/master' into 50397-refactor-lo…

273d4de

…g-filters

elasticmachine and others added 4 commits December 17, 2019 09:48

Merge branch 'master' into 50397-refactor-log-filters

783f9ab

Fix link-to test accuracy

9c39286

Merge branch '50397-refactor-log-filters' of github.com:Zacqary/kiban…

6e5c874

…a into 50397-refactor-log-filters

Fix link-to test

47ffa99

afgomez approved these changes Dec 19, 2019

View reviewed changes

weltenwort reviewed Dec 19, 2019

View reviewed changes

Zacqary added 2 commits December 19, 2019 10:56

Merge remote-tracking branch 'upstream/master' into 50397-refactor-lo…

c5e165f

…g-filters # Conflicts: # x-pack/test/functional/apps/infra/link_to.ts

Simplify destructuring signature

ef00491

Zacqary added 4 commits December 19, 2019 13:19

Stylistic fixes

8cb525d

Move URL state to hook

c61f5b1

Merge remote-tracking branch 'upstream/master' into 50397-refactor-lo…

a7fe3ef

…g-filters

Fix log filter URL state infinite loop

43302b3

Zacqary added 3 commits December 23, 2019 10:11

Merge remote-tracking branch 'upstream/master' into 50397-refactor-lo…

9063d77

…g-filters

Revert "Fix log filter URL state infinite loop"

94ae628

This reverts commit 43302b3.

Revert "Move URL state to hook"

9a7e15d

This reverts commit c61f5b1.

elastic deleted a comment from elasticmachine Dec 23, 2019

Merge remote-tracking branch 'upstream/master' into 50397-refactor-lo…

3cbf030

…g-filters

weltenwort approved these changes Jan 3, 2020

View reviewed changes

Merge branch 'master' into 50397-refactor-log-filters

ce3a832

Zacqary merged commit a03f395 into elastic:master Jan 3, 2020

This was referenced Jan 3, 2020

[7.x] [Logs UI] Refactor query bar state to hooks (#52656) #53970

Merged

[Logs UI] Refactor log position to hooks #53540

Merged

Conversation

Zacqary commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Dec 10, 2019

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

afgomez left a comment

Choose a reason for hiding this comment

Uh oh!

afgomez Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

Zacqary Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Zacqary commented Dec 17, 2019

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zacqary commented Dec 20, 2019

Uh oh!

Zacqary commented Dec 23, 2019

Uh oh!

kibanamachine commented Dec 23, 2019

💚 Build Succeeded

History

Uh oh!

kibanamachine commented Dec 23, 2019

💚 Build Succeeded

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

Zacqary commented Jan 3, 2020

Uh oh!

kibanamachine commented Jan 3, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Zacqary commented Dec 10, 2019 •

edited

Loading