-
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] Removing old angular directives #48382
[ML] Removing old angular directives #48382
Conversation
Pinging @elastic/ml-ui (:ml) |
<EuiSpacer size="m" /> | ||
<EuiCallOut | ||
title={i18n.translate('xpack.ml.accessDenied.label', { | ||
defaultMessage: 'You need permission to access ML jobs', |
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.
Compared to the previous angular version, the texts here don't mention the user roles anymore. However, the text is still specific about ML jobs, which could be irrelevant if a user doesn't have permissions to even access file visualizer. Maybe the messaging should be even more generic for this PR until we can come up with something more elaborate in combination with react-router
?
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.
good point. i've updated the text.
It's worth noting that currently this page will not be redirected to automatically. So unless someone navigates to it directly in their browser, no one will see it.
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.
One text edit need to the Callout, but otherwise LGTM
<EuiSpacer size="m" /> | ||
<EuiCallOut | ||
title={i18n.translate('xpack.ml.accessDenied.label', { | ||
defaultMessage: 'You need permission to the Ml plugin', |
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.
Should be ML
rather than Ml
to be consistent with message below.
Also title doesn't quite make sense - You need permission to use the ML plugin
or maybe Insufficient permissions
?
💔 Build Failed |
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.
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.
LGTM ⚡️
💚 Build Succeeded |
* [ML] Removing old angular directives * reverts small change * typescriptifying access denied page * changing access denied text * updating translations
* [ML] Removing old angular directives * reverts small change * typescriptifying access denied page * changing access denied text * updating translations
Removes unused angular directives from components which have been reactified.
Adjusts some component exports and import paths.
This converts the access denied page to react but this avoids any other reactification or typescriptification which would be out of scope for this PR.
Checklist
Use
strikethroughsto 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- [ ] 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