-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ML] Replacing angular routing #51842
[ML] Replacing angular routing #51842
Conversation
💔 Build Failed
|
💔 Build Failed
|
💔 Build Failed
|
💔 Build Failed
|
32afc74
to
353c62a
Compare
💔 Build Failed
|
💔 Build Failed
|
💔 Build Failed
|
💔 Build Failed
|
27e85d4
to
cf3e6f4
Compare
💔 Build Failed
|
💔 Build Failed
|
💔 Build Failed
|
💔 Build Failed
|
Pinging @elastic/ml-ui (:ml) |
@elastic/kibana-design no style changes have been made in this PR. you've been flagged because i've moved a |
9492d68
to
b29cec6
Compare
Page loading is covered by functional tests, but if we do still feel that tests for |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested latest edits, and all LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
path={route.path} | ||
exact | ||
render={props => { | ||
window.setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@walterra i just tested moving the |
* [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
* [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
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
andGlobalState
are implemented entirely. I've created a dummyAppState
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
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] 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- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately