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] use generic edit actions in the SLO embeddables #186374

Merged
merged 11 commits into from
Jun 21, 2024
Merged

[SLO] use generic edit actions in the SLO embeddables #186374

merged 11 commits into from
Jun 21, 2024

Conversation

mgiota
Copy link
Contributor

@mgiota mgiota commented Jun 18, 2024 •
edited
Loading

Fixes #186365

SLO Group Overview Embeddable

  • Edit criteria appears on top
  • Edit criteria does not appear under More actions
  • Inline Edit criteria is removed from the panel
Screen.Recording.2024-06-20.at.22.57.40.mov

SLO Alerts Embeddable

  • Edit configuration appears on top
  • Edit configuration does not appear under More actions
  • X SLOs included within the panel still opens the Edit configuration
Screen.Recording.2024-06-19.at.09.24.46.mov

@obltmachine
Copy link

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

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

@mgiota mgiota self-assigned this Jun 18, 2024
@mgiota mgiota added Team:obs-ux-management Observability Management User Experience Team v8.15.0 Feature:SLO release_note:skip Skip the PR/issue when compiling release notes labels Jun 18, 2024
@mgiota mgiota marked this pull request as ready for review June 19, 2024 06:55
@mgiota mgiota requested a review from a team as a code owner June 19, 2024 06:55
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Jun 19, 2024
mgiota
mgiota commented Jun 19, 2024
View reviewed changes
...plugins/observability_solution/slo/public/embeddable/slo/overview/slo_embeddable_factory.tsx
i18n.translate('xpack.slo.editSloOverviewEmbeddableTitle.typeDisplayName', {
defaultMessage: 'criteria',
}),
isEditingEnabled: () => api.getSloGroupOverviewConfig().overviewMode === 'groups',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ThomThomson Before I was using a custom action with an isCompatible method. Now that I removed the custom action I check only for the overviewMode to decide if editing is enabled, which is specific to the logic of the embeddable.

Do I need to do the rest checks I was doing here as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

No you don't need to do those checks anymore! Those checks were there to prevent other embeddables from ending up with the edit SLO overview action, but now that it's using a generic action that isn't a risk.

@mgiota mgiota mentioned this pull request Jun 19, 2024
Merged
6 tasks
@mgiota
Copy link
Contributor Author

mgiota commented Jun 21, 2024

@elasticmachine merge upstream

@shahzad31
Copy link
Contributor

Not sure if its this PR or it was always like this

selecting few SLOs make UI weird

image

shahzad31
shahzad31 approved these changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

Config editing still works !!

@maciejforcone
Copy link

Not sure if its this PR or it was always like this

selecting few SLOs make UI weird

Hi @mgiota @shahzad31 ,

  1. long tag in modal issue: maybe we should display SLO configuration form in the flyout as well? There will be more space, and we can wrap the long text.
  2. I think Edit and all other options can be visible right away, no second step for "settings" It's just 5 options total, so it's easy to digest for user.
image

@mgiota
Copy link
Contributor Author

mgiota commented Jun 21, 2024

  1. long tag in modal issue: maybe we should display SLO configuration form in the flyout as well? There will be more space, and we can wrap the long text.

Yep there is an issue for this already and I plan to work on this right after this one.

  1. I think Edit and all other options can be visible right away, no second step for "settings" It's just 5 options total, so it's easy to digest for user.

This is not something we handle, this is Kibana-presentation team. FYI there is an open PR hover actions, where a few actions, edit be one of them, appear without clicking on the ... button, which is pretty cool!

My main question is about the n SLOs included link within the panel and if we still want to keep it. I removed already the inline Edit criteria link from the Group Overview embeddable.

@mgiota mgiota enabled auto-merge (squash) June 21, 2024 09:26
@mgiota
Copy link
Contributor Author

mgiota commented Jun 21, 2024

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

kibana-ci commented Jun 21, 2024 •
edited
Loading

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
slo 853 850 -3

Async chunks

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

id before after diff
slo 863.4KB 866.6KB +3.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
slo 22.9KB 22.9KB +4.0B
Unknown metric groups

async chunk count

id before after diff
slo 27 29 +2

History

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

cc @mgiota

@mgiota mgiota merged commit dca0ea2 into elastic:main Jun 21, 2024
22 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 21, 2024
bhapas pushed a commit to bhapas/kibana that referenced this pull request Jun 24, 2024
@mgiota @kibanamachine @bhapas
[SLO] use generic edit actions in the SLO embeddables (elastic#186374)
5a5e057 
Fixes elastic#186365

## SLO Group Overview Embeddable
- `Edit criteria` appears on top
- Edit criteria does not appear under `More` actions
- Inline Edit criteria is removed from the panel



https://github.com/elastic/kibana/assets/2852703/4b322361-08dd-4f3f-8440-2d4380efa2bd



## SLO Alerts Embeddable
- `Edit configuration` appears on top
- Edit configuration does not appear under `More` actions
- `X SLOs included` within the panel still opens the Edit configuration



https://github.com/elastic/kibana/assets/2852703/c609fa70-4c1f-4aa5-aa17-4e765456f7e6

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThomThomson ThomThomson ThomThomson left review comments

@shahzad31 shahzad31 shahzad31 approved these changes

Assignees

@mgiota mgiota

Labels
backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project Feature:SLO release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team v8.15.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[SLO] Use generic edit actions in the SLO Embeddables
8 participants
@mgiota @obltmachine @elasticmachine @shahzad31 @maciejforcone @kibana-ci @ThomThomson @kibanamachine