Skip to content
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

Initial alerting UI and actions list #47843

Merged
merged 64 commits into from
Oct 22, 2019

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Oct 10, 2019

In this PR, we're adding a new plugin "alertingUI" that will handle the rendering of alerts and actions. The reason for a separate plugin is to still be able to render the UI when alerting plugin is disabled but actions plugin is still enabled.

This PR includes a new top level tabs that will eventually have Actions, Alerts, Activity Log and Notifications as per mock ups. This PR also includes the actions list with delete functionality.

Note: This merges into a feature branch that will later on be merged into master.

Resolves #47754
Resolves #47755

Screenshot:
Screen Shot 2019-10-17 at 10 43 21 AM

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-services (Team:Stack Services)

Copy link
Contributor

@bmcconaghy bmcconaghy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor things. I will be pulling this down and testing it for a11y and UX soon.

x-pack/legacy/plugins/alerting_ui/index.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/alerting_ui/np_ready/kibana.json Outdated Show resolved Hide resolved
*/

export const BASE_PATH = '/management/kibana/alerting';
export const BASE_ACTION_API_PATH = '../api/action';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be building this up with chrome.getBasePath() instead I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the prefix ../ in 70e8606 but I think the http service now takes care of the base path handling. It works for me while having the 3 character base path enabled.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general. I have proposal to change plugin name to AlertingUiPlugin, because the current one came up historically.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just reviewed to get familiar with what building a UI for Kibana entails, left a few nit comments.

selectMessage: i18n.translate(
'xpack.alertingUI.sections.actions.emailAction.selectMessageText',
{
defaultMessage: 'Send an email from your server.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure "from your server" is required here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is for the next upcoming PR (#48191) to create / edit actions. We decided instead of cleaning it up that we'll leave it there until the next PR lands. In order to avoid merge conflicts, we'll address this feedback in the next PR cc @YulNaumenko.

Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good; Although i believe there are many any types that could be typed; these will be very useful for future refactors.

I'm also curious why you've decided for a separate plugin for the UI instead of baking it in alerting?

Not all the i18n ids are following the naming convension outlined in this guide: https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/GUIDELINE.md I've suggested edits on many of them

<EuiSpacer size="s" />

<Switch>
{canShowActions && <Route exact path={routeToActions} component={ActionsList} />}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this but maybe canShowActions should be wrapping the <Switch>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be more routes added later on and only the one above will be related to canShowActions

@elasticmachine

This comment has been minimized.

@mikecote
Copy link
Contributor Author

@Bamieh thanks for the review. The removal of the any in TypeScript will be done with this card: https://github.com/elastic/kibana/projects/26#card-28081120. I created the card to avoid adding more merge conflicts with @YulNaumenko's upcoming PR.

The UI is developed in a separate plugin due to it containing alerting and actions plugin UI. We want to still render the UI for actions when the alerting plugin is disabled. The naming of the plugin may have to change to be more clear but this is the intent.

Thanks for the link to naming conventions, will give it a read!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mikecote mikecote requested a review from a team October 22, 2019 16:56
Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mikecote mikecote merged commit 85578a4 into feature/alerting_ui Oct 22, 2019
@mikecote
Copy link
Contributor Author

PR is marked merged automatically due to us now only using feature/alerting_ui as the single feature branch. A single PR for review into master will now be required instead of a series of PRs.

@mikecote mikecote deleted the feature/alerting_ui_plugin branch October 23, 2019 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants