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.

Sign up for GitHub

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

Jump to bottom

[AO] - Update the design of the Alert Details header and summary #153223

Merged
merged 14 commits into from
Mar 21, 2023
Merged

[AO] - Update the design of the Alert Details header and summary #153223

merged 14 commits into from
Mar 21, 2023

Conversation

fkanout
Copy link
Contributor

@fkanout fkanout commented Mar 15, 2023

Summary

It fixes #153095 and fixes #153103 by applying the new design.

Before

Screenshot 2023-03-15 at 16 26 40

After

Screenshot 2023-03-15 at 16 15 20

@fkanout fkanout requested a review from a team as a code owner March 15, 2023 15:27
@fkanout fkanout self-assigned this Mar 15, 2023
@fkanout fkanout added Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.8.0 labels Mar 15, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/actionable-observability (Team: Actionable Observability)

@fkanout fkanout added the release_note:skip Skip the PR/issue when compiling release notes label Mar 15, 2023
CoenWarmer
CoenWarmer reviewed Mar 15, 2023
View reviewed changes
Copy link
Contributor

@CoenWarmer CoenWarmer left a comment

Choose a reason for hiding this comment

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

Looks good already, just some suggestions.

x-pack/plugins/observability/public/pages/alert_details/components/page_title.tsx Outdated
Comment on lines 53 to 60
<EuiText
css={css`
font-weight: ${euiTheme.font.weight.semiBold};
`}
size="s"
>
{moment(Number(alert?.start)).fromNow()}
</EuiText>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<EuiText
css={css`
font-weight: ${euiTheme.font.weight.semiBold};
`}
size="s"
>
{moment(Number(alert?.start)).fromNow()}
</EuiText>
<EuiText
size="s"
>
<strong>{moment(Number(alert?.start)).fromNow()}</strong>
</EuiText>

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if moment().fromNow() is actually localized/translated?

Copy link
Contributor Author

@fkanout fkanout Mar 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@CoenWarmer, the semiBold we get from the euiTheme differs from the strong tag.
And I would also keep getting it from the euiTheme to keep consistency in case of theme updates/changes

Screenshot 2023-03-20 at 15 43 16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kdelemme, I think it handles it out of the box. But I added the locals that are provided by our internal lib i18n

@fkanout
Copy link
Contributor Author

fkanout commented Mar 20, 2023

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

expected head sha didn’t match current head ref.

@fkanout
Copy link
Contributor Author

fkanout commented Mar 21, 2023

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@fkanout fkanout requested review from CoenWarmer and kdelemme March 21, 2023 15:16
kdelemme
kdelemme approved these changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@kdelemme kdelemme left a comment

Choose a reason for hiding this comment

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

LGTM!

CoenWarmer
CoenWarmer approved these changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@CoenWarmer CoenWarmer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!

@fkanout fkanout enabled auto-merge (squash) March 21, 2023 15:24
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
observability 1.1MB 1.1MB -1.5KB
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3

Total ESLint disabled count

id before after diff
securitySolution 513 516 +3

History

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

cc @fkanout

@fkanout fkanout merged commit 9e1342d into elastic:main Mar 21, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CoenWarmer CoenWarmer CoenWarmer approved these changes

@kdelemme kdelemme kdelemme approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.8.0
Projects
None yet
Milestone
No milestone
6 participants
@fkanout @elasticmachine @kibanamachine @kibana-ci @CoenWarmer @kdelemme