[Discover] Deangularize navbar in context app #86353

majagrubic · 2020-12-17T18:42:21Z

Summary

The remaining piece of deangularizing context app. TopNavMenu is now a React component, rendered inside ContextAppLegacy component. The angular directive is gone 🎉

Tested in Chrome and Firefox.

Checklist

Delete any items that are not applicable to this PR.

~~- [x] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [x] Documentation was added for features that require explanation or tutorials~~

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

majagrubic · 2020-12-19T09:14:15Z

@elasticmachine merge upstream

kertal

Great that we soon can remove another brick in the angular directive wall. LGTM, except there seems to be a new error in console that should be investigated:

majagrubic · 2020-12-21T16:42:21Z

@kerta Fixed! (I think 😬 )

majagrubic · 2020-12-21T19:03:48Z

@elasticmachine merge upstream

kertal

Code LGTM 👍 , tested locally in Chrome, Firefox, Safari, no warnings, works as expected

kertal · 2020-12-22T08:21:04Z

src/plugins/discover/public/application/components/context_app/__mocks__/top_nav_menu.tsx

+
+import React from 'react';
+
+export const TopNavMenuMock = () => <div>Hello World</div>;


🌍 : hi @majagrubic

kertal · 2020-12-22T08:23:12Z

src/plugins/data/public/ui/filter_bar/filter_item.tsx

@@ -71,19 +71,27 @@ export function FilterItem(props: Props) {

  useEffect(() => {
    const index = props.filter.meta.index;
+    let isSubscribed = true;


yes, no more warning in the console! however we need to take care of rerendering of this component in a follow up PR, which is I think the reason of the former warning.

I think the reason for the warning was that getIndexPatterns is an async task, so once it's done it tries to update the state of the component, but the component has been unmounted already. The cleanup will run once the component is unmounted, cleanup will unset the flag, which will prevent subsequent updates to the state. This code is not in any way tampering with the lifecycle of the component. The only question here is if there's a more sophisticated way to get this, beyond plain flag setting, but I'll defer this decision to component owners.

lizozom

Tested on Chrome.
LGTM

lizozom · 2021-01-03T14:09:31Z

@elasticmachine merge upstream

kibanamachine · 2021-01-03T16:23:30Z

💚 Build Succeeded

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

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`data`	270.8KB	270.9KB	+105.0B
`discover`	502.3KB	502.3KB	-56.0B
total			+49.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`navigation`	20.0KB	20.0KB	+92.0B

History

💚 Build #95838 succeeded f68292d
💚 Build #95611 succeeded 1ca2330
💚 Build #95335 succeeded 1237657
💚 Build #95278 succeeded ab7bba3
💚 Build #95231 succeeded 21cd0f1

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

…meline-component * 'master' of github.com:elastic/kibana: (955 commits) remove SameSite:None workaround (elastic#86994) URL encoding for URL drilldown (elastic#86902) [Security Solution] Fix few styling issues (elastic#87045) [APM] Custom links can still be created with a read only user. (elastic#87089) prevent double update (elastic#86794) Upgrade @hapi/hoek to revert hack introduced in hapi v20 upgrade (elastic#87113) [APM] Every time the new Header Icon is clicked it fetches data (elastic#87093) [APM] Add range query to service map trace walk (elastic#86631) [Discover] Deangularize navbar in context app (elastic#86353) skip "should schedule actions on legacy alerts" elastic#87010 🍾 update notice text for 2021 [logstash] remove "upgrade" functionality now that .logstash is a system index (elastic#87056) Enable prototype pollution protection in TSVB (elastic#85952) [Security Solution] add a consistent spelling of ES in Policy Response (elastic#87073) [SECURITY_SOLUTION][ENDPOINT] Delete Endpoint Policy List code (elastic#87063) Adds more URLs to the docs links service (elastic#86972) Add missing backticks in reporting-settings.asciidoc (elastic#77979) [test/functional_cors] 9000 is sometimes in use, make getPort random (elastic#87050) [Security Solution] Fix Timeline filter EuiSuperSelect styling (elastic#87033) [Lens] Fix duplicate suggestions on single-bucket charts (elastic#86996) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/index.ts

* [Discover] Deangualrize navbar in context * Remove directive * Updating a unit test * Updating a unit test * Add cleanup in FilterItem component Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

[Discover] Deangualrize navbar in context

21cd0f1

majagrubic changed the title ~~[Discover] Deangualrize navbar in context~~ [Discover] Deangualrize navbar in context app Dec 17, 2020

Remove directive

ab7bba3

majagrubic added v7.12.0 v8.0.0 Feature:Discover Discover Application release_note:roadmap labels Dec 18, 2020

majagrubic changed the title ~~[Discover] Deangualrize navbar in context app~~ [Discover] Deangularize navbar in context app Dec 18, 2020

majagrubic marked this pull request as ready for review December 18, 2020 07:22

majagrubic requested a review from a team December 18, 2020 07:22

majagrubic requested a review from a team as a code owner December 18, 2020 07:22

majagrubic requested review from kertal and lizozom December 18, 2020 07:22

Maja Grubic added 2 commits December 18, 2020 07:31

Updating a unit test

da93195

Updating a unit test

1237657

Merge branch 'master' into deangularize-navbar

1ca2330

kertal reviewed Dec 21, 2020

View reviewed changes

kibanamachine and others added 2 commits December 21, 2020 14:03

Merge branch 'master' into deangularize-navbar

e08a704

Add cleanup in FilterItem component

f68292d

kertal approved these changes Dec 22, 2020

View reviewed changes

lizozom approved these changes Jan 3, 2021

View reviewed changes

Merge branch 'master' into deangularize-navbar

9a0b521

majagrubic merged commit 8d2e51d into elastic:master Jan 4, 2021

majagrubic deleted the deangularize-navbar branch January 4, 2021 07:41

majagrubic mentioned this pull request Jan 4, 2021

[7.x] [Discover] Deangularize navbar in context app (#86353) #87111

Merged

timroes added release_note:enhancement and removed release_note:roadmap labels Mar 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Deangularize navbar in context app #86353

[Discover] Deangularize navbar in context app #86353

majagrubic commented Dec 17, 2020 •

edited

Loading

majagrubic commented Dec 19, 2020

kertal left a comment

majagrubic commented Dec 21, 2020 •

edited

Loading

majagrubic commented Dec 21, 2020

kertal left a comment

kertal Dec 22, 2020

kertal Dec 22, 2020

majagrubic Dec 22, 2020 •

edited

Loading

lizozom left a comment

lizozom commented Jan 3, 2021

kibanamachine commented Jan 3, 2021


		import React from 'react';

		export const TopNavMenuMock = () => <div>Hello World</div>;

[Discover] Deangularize navbar in context app #86353

[Discover] Deangularize navbar in context app #86353

Conversation

majagrubic commented Dec 17, 2020 • edited Loading

Summary

Checklist

For maintainers

majagrubic commented Dec 19, 2020

kertal left a comment

Choose a reason for hiding this comment

majagrubic commented Dec 21, 2020 • edited Loading

majagrubic commented Dec 21, 2020

kertal left a comment

Choose a reason for hiding this comment

kertal Dec 22, 2020

Choose a reason for hiding this comment

kertal Dec 22, 2020

Choose a reason for hiding this comment

majagrubic Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

lizozom left a comment

Choose a reason for hiding this comment

lizozom commented Jan 3, 2021

kibanamachine commented Jan 3, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

majagrubic commented Dec 17, 2020 •

edited

Loading

majagrubic commented Dec 21, 2020 •

edited

Loading

majagrubic Dec 22, 2020 •

edited

Loading