[ML] Replacing angular routing #51842

jgowdyelastic · 2019-11-27T19:01:50Z

Replaces angular routing layer with a react routing layer.
To aid transition to the new routing type I've copied angular's idea of having a resolving step before each page load. As before, this takes a collection of promise returning functions.

AppState and GlobalState are implemented entirely. I've created a dummy AppState class to be passed into the observables so not to change them, but any updates to the page are not reflected in the URL.
On page load the _g and _a are parsed and passed to the page as before.

Saved searches are now saved objects, rather than a SavedSearch object from Discover.
IndexPatterns are injected from the data plugin.

There are still more includes that need to be shimmed, but this will happen in follow up PRs.

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-11-27T20:18:31Z

💔 Build Failed

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

elasticmachine · 2019-11-27T22:27:52Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 4cc209e

elasticmachine · 2019-11-28T16:23:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 0c7849a

elasticmachine · 2019-11-28T20:41:27Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 32afc74

elasticmachine · 2019-12-02T13:04:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 353c62a

elasticmachine · 2019-12-02T14:07:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 96bcd72

elasticmachine · 2019-12-02T15:38:01Z

💔 Build Failed

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

elasticmachine · 2019-12-02T21:23:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 7781e42

elasticmachine · 2019-12-04T17:25:47Z

💔 Build Failed

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

elasticmachine · 2019-12-04T18:02:00Z

💔 Build Failed

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

elasticmachine · 2019-12-05T18:58:30Z

💔 Build Failed

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

elasticmachine · 2019-12-05T21:36:21Z

💔 Build Failed

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

elasticmachine · 2019-12-06T10:50:59Z

Pinging @elastic/ml-ui (:ml)

jgowdyelastic · 2019-12-06T11:21:47Z

@elastic/kibana-design no style changes have been made in this PR. you've been flagged because i've moved a scss file.

jgowdyelastic · 2019-12-10T11:58:29Z

Because the directives are gone, this PR removes the /test/directive. files. It would be great if the new corresponding PageWrapper components had a similar jest test.

Page loading is covered by functional tests, but if we do still feel that tests for PageWrapper would be useful I can add them in a follow up.
I think they will not be trivial due to the amount of mocking that will be needed for the resolvers.

elasticmachine · 2019-12-10T13:06:26Z

💚 Build Succeeded

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

History

💚 Build #14295 succeeded 9492d68
💔 Build #14273 failed 2c1c20c
💚 Build #14245 succeeded da6e5a9
💚 Build #14180 succeeded f29478c
💚 Build #14145 succeeded 7a0f92e

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

peteharverson

Tested latest edits, and all LGTM.

walterra

Latest changes LGTM. I left another comment about the setTimeout(), it might be worth revisiting in a follow up. Regarding the mocha tests: You're right, let's skip those jest tests. The directive mocha tests were run in a browser environment just to check if everything would load without errors which is now covered by functional tests like you said.

walterra · 2019-12-11T13:25:03Z

x-pack/legacy/plugins/ml/public/application/routing/router.tsx

+            path={route.path}
+            exact
+            render={props => {
+              window.setTimeout(() => {


maybe something for a follow up: Does render= take a component? If so maybe the side effect could be done within here using useEffect(() => { setBreamcrumbs(route.breadcrumbs); }, []); - we had a similar discussion around the EuiDataGrid which uses a similar approach, have a look for renderCellValue in the data grid docs here: https://elastic.github.io/eui/#/tabular-content/data-grid

When using setTimeout() for a similar case with data grid we found an issue where the callback for the timeout could trigger an error if the wrapping component got unmounted while the callback was called.

jgowdyelastic · 2019-12-11T14:03:48Z

@walterra i just tested moving the setBreadcrumbs call to a useEffect inside each PageWrapper and it works! great idea!
It means setBreadcrumbs needs to be passed down through the render function, but that is fine.
As this PR is green and has been reviewed, I'd suggest I make this change in a follow up PR.

* [ML] Replacing angular routing * removing old files * changing overview * renaming overview route * adding df analytics routes * adding timeseriesexplorer route * removing old files * adding route for explorer * adding access denied page * adding module view or create redirect * fixing job cloning * adding breadcrumb system * removing old breadcrumbs files * fix include * enabling management section * injecting app dependencies * fixing missed dependencies * fixing saved searches * fixing type errors * removing included data start * code clean up * updating translations * fixing router test failures * fixing functional tests * removing last use of SavedSearch * removing comment * fixing bug in line chart query * improving saved search jobs * fixing data viz functional test * adding comment * dealing with time range error * removing unnecessary chrome imports * cleaning up code * moving resolver to own file * changes based on review * fixing index data viz on basic license * fixing edit calendar * adding create job breadcrumb * fixing results appstate * fixing management links * updating new job constants file * fixing rebase conflicts * removing commented out code * adding additional text to the resolver error

jgowdyelastic force-pushed the replacing-angular-routing branch from 32afc74 to 353c62a Compare December 2, 2019 11:54

jgowdyelastic force-pushed the replacing-angular-routing branch from 27e85d4 to cf3e6f4 Compare December 4, 2019 16:00

jgowdyelastic marked this pull request as ready for review December 6, 2019 10:50

jgowdyelastic requested review from a team as code owners December 6, 2019 10:50

jgowdyelastic requested review from peteharverson, walterra, darnautov and alvarezmelissa87 December 6, 2019 10:50

jgowdyelastic self-assigned this Dec 6, 2019

jgowdyelastic added the :ml label Dec 6, 2019

jgowdyelastic added non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes review v7.6.0 v8.0.0 labels Dec 6, 2019

jgowdyelastic added 14 commits December 10, 2019 11:52

dealing with time range error

61bae64

removing unnecessary chrome imports

59dc845

cleaning up code

c147203

moving resolver to own file

30dc678

changes based on review

0099117

fixing index data viz on basic license

419a61a

fixing edit calendar

16aea2b

adding create job breadcrumb

8c78e09

fixing results appstate

acfd82a

fixing management links

dd39090

updating new job constants file

fa91ad7

fixing rebase conflicts

e720e35

removing commented out code

f4a3d33

adding additional text to the resolver error

b29cec6

jgowdyelastic force-pushed the replacing-angular-routing branch from 9492d68 to b29cec6 Compare December 10, 2019 11:53

peteharverson approved these changes Dec 11, 2019

View reviewed changes

walterra approved these changes Dec 11, 2019

View reviewed changes

jgowdyelastic merged commit 4f2a6f8 into elastic:master Dec 11, 2019

jgowdyelastic mentioned this pull request Dec 11, 2019

[7.x] [ML] Replacing angular routing (#51842) #52770

Merged

jgowdyelastic deleted the replacing-angular-routing branch December 11, 2019 17:49

peteharverson added the Feature:NP Migration label Dec 12, 2019

walterra mentioned this pull request Dec 13, 2019

[ML] Fix appState/globalState #52986

Closed

21 tasks

This was referenced Jan 17, 2020

[ML] (Accessibility) Multiple Accessibility Best Practice Issues on Machine Learning Overview Page - MAIN Landmark #51076

Closed

[ML] (Accessibility) Machine Learning - Anomaly Detection / Create Job - Landmark Accessibility Best Practice Issue #52377

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Replacing angular routing #51842

[ML] Replacing angular routing #51842

jgowdyelastic commented Nov 27, 2019 •

edited

Loading

elasticmachine commented Nov 27, 2019

elasticmachine commented Nov 27, 2019

elasticmachine commented Nov 28, 2019

elasticmachine commented Nov 28, 2019

elasticmachine commented Dec 2, 2019

elasticmachine commented Dec 2, 2019

elasticmachine commented Dec 2, 2019

elasticmachine commented Dec 2, 2019

elasticmachine commented Dec 4, 2019

elasticmachine commented Dec 4, 2019

elasticmachine commented Dec 5, 2019

elasticmachine commented Dec 5, 2019

elasticmachine commented Dec 6, 2019

jgowdyelastic commented Dec 6, 2019 •

edited

Loading

jgowdyelastic commented Dec 10, 2019 •

edited

Loading

elasticmachine commented Dec 10, 2019

peteharverson left a comment

walterra left a comment

walterra Dec 11, 2019

jgowdyelastic commented Dec 11, 2019

[ML] Replacing angular routing #51842

[ML] Replacing angular routing #51842

Conversation

jgowdyelastic commented Nov 27, 2019 • edited Loading

Checklist

For maintainers

elasticmachine commented Nov 27, 2019

💔 Build Failed

elasticmachine commented Nov 27, 2019

💔 Build Failed

elasticmachine commented Nov 28, 2019

💔 Build Failed

elasticmachine commented Nov 28, 2019

💔 Build Failed

elasticmachine commented Dec 2, 2019

💔 Build Failed

elasticmachine commented Dec 2, 2019

💔 Build Failed

elasticmachine commented Dec 2, 2019

💔 Build Failed

elasticmachine commented Dec 2, 2019

💔 Build Failed

elasticmachine commented Dec 4, 2019

💔 Build Failed

elasticmachine commented Dec 4, 2019

💔 Build Failed

elasticmachine commented Dec 5, 2019

💔 Build Failed

elasticmachine commented Dec 5, 2019

💔 Build Failed

elasticmachine commented Dec 6, 2019

jgowdyelastic commented Dec 6, 2019 • edited Loading

jgowdyelastic commented Dec 10, 2019 • edited Loading

elasticmachine commented Dec 10, 2019

💚 Build Succeeded

History

peteharverson left a comment

Choose a reason for hiding this comment

walterra left a comment

Choose a reason for hiding this comment

walterra Dec 11, 2019

Choose a reason for hiding this comment

jgowdyelastic commented Dec 11, 2019

jgowdyelastic commented Nov 27, 2019 •

edited

Loading

jgowdyelastic commented Dec 6, 2019 •

edited

Loading

jgowdyelastic commented Dec 10, 2019 •

edited

Loading