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

[SLO][SLO Detail] Make SLO Detail > Header Actions menu and SLO List > SLO List item > Actions menu more consistent #155868

Merged
merged 11 commits into from
Apr 26, 2023
Merged

[SLO][SLO Detail] Make SLO Detail > Header Actions menu and SLO List > SLO List item > Actions menu more consistent #155868

merged 11 commits into from
Apr 26, 2023

Conversation

CoenWarmer
Copy link
Contributor

Resolves #155849

📝 Summary

This makes the SLO Details > Header Actions menu consistent with the SLO List item > Actions menu.

Screen.Recording.2023-04-26.at.14.56.01.mov

✅ Checklist

  • The SLO Detail > Header actions menu should also have "Clone"
  • The SLO Detail > Header actions menu should also have "Delete"
  • Both menus should use "medium" padding
  • Both menus should use the gear icon for "Manage rules"
  • The SLO Detail > Header actions menu should also have icons, same as SLO List item
  • The SLO Detail > Header actions menu > Create new Alert rule option should have the same capitalization as on SLO List Item > Actions menu option

@CoenWarmer CoenWarmer requested a review from a team as a code owner April 26, 2023 12:58
@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@CoenWarmer CoenWarmer added the release_note:skip Skip the PR/issue when compiling release notes label Apr 26, 2023
@CoenWarmer CoenWarmer changed the title Add disabled state to Feedback button [SLO][SLO Detail] Make SLO Detail > Header Actions menu and SLO List > SLO List item > Actions menu more consistent Apr 26, 2023
kdelemme
kdelemme approved these changes Apr 26, 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.

Nice improvements, LGTM 👍🏻

x-pack/plugins/observability/public/pages/slo_details/components/header_control.tsx Outdated Show resolved Hide resolved
kdelemme
kdelemme requested changes Apr 26, 2023
View reviewed changes
Copy link
Contributor

@kdelemme kdelemme 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.

One thing I just notice is the react errors in the console:

  • When cloning from the slo details page
  • When deleting from the slo details page

It does not happen all the time, maybe because the component unmount after the state is updated, but sometime it does not resulting in a "cannot update state after component unmounted":
image

@CoenWarmer CoenWarmer mentioned this pull request Apr 26, 2023
Closed
@CoenWarmer
Copy link
Contributor Author

One thing I just notice is the react errors in the console:

  • When cloning from the slo details page
  • When deleting from the slo details page

It does not happen all the time, maybe because the component unmount after the state is updated, but sometime it does not resulting in a "cannot update state after component unmounted": image

@kdelemme Noticed them too. Created #155887. Will attempt to fix them for 8.8.0.

@CoenWarmer CoenWarmer requested a review from kdelemme April 26, 2023 14:29
kdelemme
kdelemme approved these changes Apr 26, 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.

Approving since the Errors are warning that won't be displayed in production bundle. But let's try to fix this, I'm not a huge fan of shipping known errors after FF (even if this one is small, and won't cause real issue)

@CoenWarmer CoenWarmer enabled auto-merge (squash) April 26, 2023 16:20
@CoenWarmer CoenWarmer merged commit 1dcf1f8 into elastic:main Apr 26, 2023
@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 964.3KB 965.5KB +1.2KB
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 17 19 +2
securitySolution 399 402 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 18 20 +2
securitySolution 479 482 +3
total +5

History

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

@kibanamachine kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Apr 26, 2023
@CoenWarmer
Copy link
Contributor Author

CoenWarmer commented Apr 26, 2023 •
edited
Loading

Approving since the Errors are warning that won't be displayed in production bundle. But let's try to fix this, I'm not a huge fan of shipping known errors after FF (even if this one is small, and won't cause real issue)

Agree and same.

Created #155953 to fix.

@CoenWarmer CoenWarmer added backport and removed backport:skip This commit does not require backporting labels Apr 28, 2023
CoenWarmer added a commit to CoenWarmer/kibana that referenced this pull request Apr 28, 2023
@CoenWarmer
[SLO][SLO Detail] Make SLO Detail > Header Actions menu and SLO List …
e6df4b5 
…> SLO List item > Actions menu more consistent (elastic#155868)

Co-authored-by: Kevin Delemme <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit 1dcf1f8)

# Conflicts:
#	x-pack/plugins/observability/public/pages/slo_details/components/header_control.tsx
#	x-pack/plugins/observability/public/pages/slo_details/slo_details.test.tsx
@CoenWarmer CoenWarmer mentioned this pull request Apr 28, 2023
Merged
@CoenWarmer
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
✅ 8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

CoenWarmer added a commit that referenced this pull request Apr 28, 2023
@CoenWarmer
[8.8] [SLO][SLO Detail] Make SLO Detail > Header Actions menu and SLO…
8a0eea0 
… List > SLO List item > Actions menu more consistent (#155868) (#156157)

# Backport

This will backport the following commits from `main` to `8.8`:
- [[SLO][SLO Detail] Make SLO Detail > Header Actions menu and SLO List
> SLO List item > Actions menu more consistent
(#155868)](#155868)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Coen
Warmer","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-04-26T17:20:43Z","message":"[SLO][SLO
Detail] Make SLO Detail > Header Actions menu and SLO List > SLO List
item > Actions menu more consistent (#155868)\n\nCo-authored-by: Kevin
Delemme <[email protected]>\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"1dcf1f86a856a7251df7bd06b47820ed64d033b8","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["backport","release_note:skip","v8.8.0"],"number":155868,"url":"https://github.com/elastic/kibana/pull/155868","mergeCommit":{"message":"[SLO][SLO
Detail] Make SLO Detail > Header Actions menu and SLO List > SLO List
item > Actions menu more consistent (#155868)\n\nCo-authored-by: Kevin
Delemme <[email protected]>\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"1dcf1f86a856a7251df7bd06b47820ed64d033b8"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/155868","number":155868,"mergeCommit":{"message":"[SLO][SLO
Detail] Make SLO Detail > Header Actions menu and SLO List > SLO List
item > Actions menu more consistent (#155868)\n\nCo-authored-by: Kevin
Delemme <[email protected]>\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"1dcf1f86a856a7251df7bd06b47820ed64d033b8"}}]}]
BACKPORT-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kdelemme kdelemme kdelemme approved these changes

Assignees
No one assigned
Labels
backport release_note:skip Skip the PR/issue when compiling release notes v8.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[SLO][SLO Detail] Header Actions menu should offer the same options as SLO List Item > Actions menu
5 participants
@CoenWarmer @apmmachine @kibana-ci @kdelemme @kibanamachine