[Stack Monitoring] Add setup mode to react app #110670

estermv · 2021-08-31T16:38:47Z

Summary

This PR adds the setup mode to the react app.
I just tested it in the overview page. To verify that is working I enabled the legacy monitoring, so this badge appears when you enter the setup mode:

Notes for the reviewer

Set monitoring.ui.render_react_app: true in kibana.dev.yml to render the react app.
SetupModeRenderer and setup_mode.tsx are almost copies from the original files, just adapting them to work in react. When angular is removed, we will need to move setup_mode.tsx to the original location so all imports from existing react child components keep working.

matschaffer · 2021-09-01T07:05:42Z

x-pack/plugins/monitoring/public/application/setup_mode/setup_mode_renderer.js

+ * 2.0.
+ */
+
+import React, { Fragment } from 'react';


This seems like it's forked from https://github.com/elastic/kibana/blob/master/x-pack/plugins/monitoring/public/components/renderers/setup_mode.js

Do you think we could use the legacy shims to avoid the fork? Or if not should we maybe also fork https://github.com/elastic/kibana/blob/master/x-pack/plugins/monitoring/public/components/renderers/setup_mode.test.js ?

Yes, it's forked from that file. I couldn't use the legacy shims because there is no scope mock in the Legacy class and because the global state context doesn't exist in the Angular work and it's needed here.

But good point with forking the tests, I'll take a look now to see if it's easy to adapt them to the changes made 🤞

I didn't fork tests yet. There are some snapshot tests that I think it would be interesting to run against (I could also copy that, but I think is too much copy).

matschaffer

I did some exploring to try to figure out why the alerts from the angular side weren't showing in the new setup mode. But looks like they get wired up as part of the base_controller.js, so that explains it.

Otherwise looks workable. Curious about that forked code, but not curious enough to block merge.

elasticmachine · 2021-09-02T15:50:57Z

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

estermv · 2021-09-02T15:53:29Z

x-pack/plugins/monitoring/public/lib/setup_mode.tsx

@@ -209,6 +211,7 @@ export const initSetupModeState = async ($scope: any, $injector: any, callback?:
 };

 export const isInSetupMode = (context?: ISetupModeContext) => {
+  if (isReactMigrationEnabled()) return setupModeReact.isInSetupMode(context);


This method is used in many places in react child components rendered also in angular. This is a hack to make it work with the new file without modifying several components.

estermv · 2021-09-02T15:53:37Z

x-pack/plugins/monitoring/public/lib/setup_mode.tsx

@@ -222,6 +225,7 @@ export const isInSetupMode = (context?: ISetupModeContext) => {
 };

 export const isSetupModeFeatureEnabled = (feature: SetupModeFeature) => {
+  if (isReactMigrationEnabled()) return setupModeReact.isSetupModeFeatureEnabled(feature);


This method is used in many places in react child components rendered also in angular. This is a hack to make it work with the new file without modifying several components.

estermv · 2021-09-02T15:54:45Z

@matschaffer I added some changes since you reviewed, so I requested another review.

estermv · 2021-09-02T16:01:57Z

@elasticmachine merge upstream

matschaffer

I love the isInSetupMode refactoring update. But I noticed we have duplicate buttons.

We could merge this and sort that out in a later PR but maybe good to fix up now.

matschaffer · 2021-09-03T04:38:38Z

x-pack/plugins/monitoring/public/application/setup_mode/setup_mode.tsx

+        <SetupModeEnterButton enabled={enabled} toggleSetupMode={toggleSetupMode} />
+      </I18nContext>
+    </KibanaContextProvider>,
+    document.getElementById('setupModeNav')


Why do we renter into the div here rather than put a conditional component into the PageTemplate?

Also I noticed we now have two "setup" things, so guessing we should maybe move the setupModeNav into MonitoringToolbar?

This is how it was done before and I don't know exactly why they did it that way.

When I added it, it just worked so I thought that changing it will take more time and it could introduce some bugs. I also think that everything related to setup mode needs to be refactored, both from a UX perspective (I'm not sure how intuitive it is for users...) and technically so we could improve everything together.

estermv · 2021-09-03T08:29:33Z

@elasticmachine merge upstream

kibanamachine · 2021-09-03T13:12:14Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`monitoring`	580	583	+3

Async chunks

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

id	before	after	diff
`monitoring`	762.6KB	775.4KB	+12.8KB

Page load bundle

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

id	before	after	diff
`monitoring`	47.9KB	48.3KB	+439.0B

History

💔 Build #150860 failed 3961c51
💚 Build #150847 succeeded 3ed7ab9
💚 Build #150641 succeeded 15e9184
💔 Build #150629 failed 1759c25
💔 Build #150566 failed e50ac54

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

kibanamachine · 2021-09-03T13:20:10Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 110670

kibanamachine · 2021-09-07T13:21:52Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 110670 or prevent reminders by adding the backport:skip label.

* Show setup mode button and setup bottom bar * Adapt setup mode in react components to work without angular * Add setup mode data update to react app * Add missing functions from setup mode * Revert setup mode changes from react components * remove some empty lines * Add setup button to monitoring toolbar * Fix types Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Show setup mode button and setup bottom bar * Adapt setup mode in react components to work without angular * Add setup mode data update to react app * Add missing functions from setup mode * Revert setup mode changes from react components * remove some empty lines * Add setup button to monitoring toolbar * Fix types Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

estermv added 2 commits August 31, 2021 18:28

Show setup mode button and setup bottom bar

e88232b

Adapt setup mode in react components to work without angular

1f57167

matschaffer reviewed Sep 1, 2021

View reviewed changes

matschaffer approved these changes Sep 1, 2021

View reviewed changes

estermv added 4 commits September 2, 2021 13:16

Add setup mode data update to react app

7a64971

Add missing functions from setup mode

e50ac54

Revert setup mode changes from react components

aa4fcbb

remove some empty lines

1759c25

estermv marked this pull request as ready for review September 2, 2021 15:50

estermv requested a review from a team as a code owner September 2, 2021 15:50

estermv requested review from a team September 2, 2021 15:50

estermv commented Sep 2, 2021

View reviewed changes

estermv requested a review from matschaffer September 2, 2021 15:53

Merge branch 'master' into add-setup-mode

15e9184

matschaffer reviewed Sep 3, 2021

View reviewed changes

kibanamachine and others added 2 commits September 3, 2021 04:29

Merge branch 'master' into add-setup-mode

3ed7ab9

Add setup button to monitoring toolbar

3961c51

Fix types

eb02b51

estermv merged commit 75486ec into elastic:master Sep 3, 2021

matschaffer mentioned this pull request Sep 7, 2021

[Stack monitoring] Convert Angular views to React #111309

Closed

19 tasks

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 7, 2021

estermv mentioned this pull request Sep 7, 2021

[7.x] [Stack Monitoring] Add setup mode to react app (#110670) #111395

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Stack Monitoring] Add setup mode to react app #110670

[Stack Monitoring] Add setup mode to react app #110670

estermv commented Aug 31, 2021 •

edited

Loading

matschaffer Sep 1, 2021

estermv Sep 2, 2021

estermv Sep 2, 2021

matschaffer left a comment

elasticmachine commented Sep 2, 2021

estermv Sep 2, 2021

estermv Sep 2, 2021

estermv commented Sep 2, 2021

estermv commented Sep 2, 2021

matschaffer left a comment

matschaffer Sep 3, 2021

estermv Sep 3, 2021

estermv commented Sep 3, 2021

kibanamachine commented Sep 3, 2021

kibanamachine commented Sep 3, 2021

kibanamachine commented Sep 7, 2021

[Stack Monitoring] Add setup mode to react app #110670

[Stack Monitoring] Add setup mode to react app #110670

Conversation

estermv commented Aug 31, 2021 • edited Loading

Summary

Notes for the reviewer

matschaffer Sep 1, 2021

Choose a reason for hiding this comment

estermv Sep 2, 2021

Choose a reason for hiding this comment

estermv Sep 2, 2021

Choose a reason for hiding this comment

matschaffer left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 2, 2021

estermv Sep 2, 2021

Choose a reason for hiding this comment

estermv Sep 2, 2021

Choose a reason for hiding this comment

estermv commented Sep 2, 2021

estermv commented Sep 2, 2021

matschaffer left a comment

Choose a reason for hiding this comment

matschaffer Sep 3, 2021

Choose a reason for hiding this comment

estermv Sep 3, 2021

Choose a reason for hiding this comment

estermv commented Sep 3, 2021

kibanamachine commented Sep 3, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

kibanamachine commented Sep 3, 2021

💔 Backport failed

kibanamachine commented Sep 7, 2021

estermv commented Aug 31, 2021 •

edited

Loading