Skip to content

[Response Ops][Rules] Settings as Flyout instead of Modal#216162

Merged
jcger merged 10 commits intoelastic:mainfrom
jcger:issue-215910-rule-settings-in-flyout
Apr 1, 2025
Merged

[Response Ops][Rules] Settings as Flyout instead of Modal#216162
jcger merged 10 commits intoelastic:mainfrom
jcger:issue-215910-rule-settings-in-flyout

Conversation

@jcger
Copy link
Contributor

@jcger jcger commented Mar 27, 2025 •
edited
Loading

Summary

Closes #215910

Screenshot 2025-03-28 at 12 28 08

Release note:

Moves rule settings to a flyout instead of a modal

@jcger jcger added Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// ci:cloud-deploy Create or update a Cloud deployment release_note:enhancement v9.1.0 v8.19.0 labels Mar 28, 2025
@jcger jcger marked this pull request as ready for review March 28, 2025 11:33
@jcger jcger requested a review from a team as a code owner March 28, 2025 11:33
@elasticmachine
Copy link
Contributor

elasticmachine commented Mar 28, 2025

Pinging @elastic/response-ops (Team:ResponseOps)

@jcger jcger added the backport:skip This PR does not require backporting label Mar 28, 2025
@jcger jcger requested review from Zacqary and adcoelho March 28, 2025 11:38
adcoelho
adcoelho reviewed Mar 31, 2025
View reviewed changes
...iggers_actions_ui/public/application/components/rules_setting/rules_settings_flyout.test.tsx
});

test('can save flapping settings', async () => {
const result = render(<RulesSettingsFlyoutWithProviders {...flyoutProps} />);
Copy link
Contributor

@adcoelho adcoelho Mar 31, 2025

Choose a reason for hiding this comment

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

Suggested change
const result = render(<RulesSettingsFlyoutWithProviders {...flyoutProps} />);
render(<RulesSettingsFlyoutWithProviders {...flyoutProps} />);

nit: screen is preferred for querying

Copy link
Contributor Author

@jcger jcger Mar 31, 2025

Choose a reason for hiding this comment

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

Sorry, somehow I created a copy of the this file before renaming it and github thought this was a new file. Regarding using screen, as there is not just this test that would need to be updated I prefer to do so in another PR dedicated to that and not mix things up

Copy link
Contributor

@adcoelho adcoelho Mar 31, 2025

Choose a reason for hiding this comment

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

I prefer to do so in another PR dedicated to that and not mix things up

yy no worries.

and github thought this was a new file.

I was actually wondering if this was the case because they looked similar to the other tests.

adcoelho
adcoelho reviewed Mar 31, 2025
View reviewed changes
...iggers_actions_ui/public/application/components/rules_setting/rules_settings_flyout.test.tsx
Comment on lines 165 to 168
await waitForFlyoutLoad();
expect(
result.getByTestId('rulesSettingsFlappingEnableSwitch').getAttribute('aria-checked')
).toBe('true');
Copy link
Contributor

@adcoelho adcoelho Mar 31, 2025

Choose a reason for hiding this comment

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

This is all a consequence of debugging too many cases' flaky tests ™️ but I got used to await screen.findBy* everywhere(vs getBy*).

I think it's fine here anyway, but I was wondering the following:

Suggested change
await waitForFlyoutLoad();
expect(
result.getByTestId('rulesSettingsFlappingEnableSwitch').getAttribute('aria-checked')
).toBe('true');
expect(
(await screen.findByTestId('rulesSettingsFlappingEnableSwitch')).getAttribute('aria-checked')
).toBe('true');

would this work/be simpler?

@elasticmachine
Copy link
Contributor

elasticmachine commented Mar 31, 2025 •
edited
Loading

💚 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
apm 2.5MB 2.5MB +15.0B
discover 955.3KB 955.3KB +15.0B
infra 1.2MB 1.2MB +15.0B
ml 5.4MB 5.4MB +15.0B
monitoring 642.8KB 642.8KB +15.0B
observability 1.3MB 1.3MB +15.0B
slo 929.5KB 929.5KB +15.0B
synthetics 988.0KB 988.0KB +15.0B
transform 634.7KB 634.7KB +15.0B
triggersActionsUi 1.5MB 1.5MB +265.0B
uptime 533.5KB 533.5KB +15.0B
total +415.0B

History

Zacqary
Zacqary approved these changes Mar 31, 2025
View reviewed changes
Copy link
Contributor

@Zacqary Zacqary left a comment

Choose a reason for hiding this comment

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

LGTM

@jcger jcger merged commit 2881895 into elastic:main Apr 1, 2025
9 checks passed
This was referenced Apr 9, 2025
Merged
Merged
@kibanamachine kibanamachine mentioned this pull request Apr 16, 2025
Merged
@jcger jcger mentioned this pull request Jun 9, 2025
Merged
@jcger
Copy link
Contributor Author

jcger commented Jun 9, 2025

💚 All backports created successfully

Status Branch Result
✅ 8.19

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

Questions ?

Please refer to the Backport tool documentation

jcger added a commit to jcger/kibana that referenced this pull request Jun 9, 2025
@jcger
[Response Ops][Rules] Settings as Flyout instead of Modal (elastic#21…
d79f106 
…6162)

## Summary

Closes elastic#215910

<img width="597" alt="Screenshot 2025-03-28 at 12 28 08"
src="https://github.com/user-attachments/assets/6f4b5cb0-0778-4771-851a-a0a2d295f6b1"
/>

## Release note:
Moves rule settings to a flyout instead of a modal

(cherry picked from commit 2881895)
jcger added a commit that referenced this pull request Jun 9, 2025
@jcger
[8.19] [Response Ops][Rules] Settings as Flyout instead of Modal (#21…
7844c1f 
…6162) (#223104)

# Backport

This will backport the following commits from `main` to `8.19`:
- [[Response Ops][Rules] Settings as Flyout instead of Modal
(#216162)](#216162)

<!--- Backport version: 10.0.0 -->

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

<!--BACKPORT [{"author":{"name":"Julian
Gernun","email":"17549662+jcger@users.noreply.github.com"},"sourceCommit":{"committedDate":"2025-04-01T07:59:25Z","message":"[Response
Ops][Rules] Settings as Flyout instead of Modal (#216162)\n\n##
Summary\n\nCloses
https://github.com/elastic/kibana/issues/215910\n\n<img width=\"597\"
alt=\"Screenshot 2025-03-28 at 12 28
08\"\nsrc=\"https://github.com/user-attachments/assets/6f4b5cb0-0778-4771-851a-a0a2d295f6b1\"\n/>\n\n##
Release note:\nMoves rule settings to a flyout instead of a
modal","sha":"2881895b4531653859ae1fd8515d17297d85dc81","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","backport:skip","Team:ResponseOps","ci:cloud-deploy","v9.1.0","v8.19.0"],"title":"[Response
Ops][Rules] Settings as Flyout instead of
Modal","number":216162,"url":"https://github.com/elastic/kibana/pull/216162","mergeCommit":{"message":"[Response
Ops][Rules] Settings as Flyout instead of Modal (#216162)\n\n##
Summary\n\nCloses
https://github.com/elastic/kibana/issues/215910\n\n<img width=\"597\"
alt=\"Screenshot 2025-03-28 at 12 28
08\"\nsrc=\"https://github.com/user-attachments/assets/6f4b5cb0-0778-4771-851a-a0a2d295f6b1\"\n/>\n\n##
Release note:\nMoves rule settings to a flyout instead of a
modal","sha":"2881895b4531653859ae1fd8515d17297d85dc81"}},"sourceBranch":"main","suggestedTargetBranches":["8.19"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/216162","number":216162,"mergeCommit":{"message":"[Response
Ops][Rules] Settings as Flyout instead of Modal (#216162)\n\n##
Summary\n\nCloses
https://github.com/elastic/kibana/issues/215910\n\n<img width=\"597\"
alt=\"Screenshot 2025-03-28 at 12 28
08\"\nsrc=\"https://github.com/user-attachments/assets/6f4b5cb0-0778-4771-851a-a0a2d295f6b1\"\n/>\n\n##
Release note:\nMoves rule settings to a flyout instead of a
modal","sha":"2881895b4531653859ae1fd8515d17297d85dc81"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adcoelho adcoelho adcoelho left review comments

@Zacqary Zacqary Zacqary approved these changes

Assignees

No one assigned

Labels

backport:skip This PR does not require backporting ci:cloud-deploy Create or update a Cloud deployment release_note:enhancement Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v8.19.0 v9.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Response Ops][Alerting] Move rule settings to a flyout instead of a modal

4 participants

@jcger @elasticmachine @Zacqary @adcoelho