[Security Solution] Reduce dropdown options and improve tooltip texts in Rule Upgrade flyout #203222
+365
−165
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nikitaindik
added
release_note:skip
nikitaindik
force-pushed
the
better-popup-texts
branch
from
963fb56 to
3f29e79
Compare
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
|
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
nikitaindik
commented
Dec 6, 2024
| id="xpack.securitySolution.detectionEngine.rules.upgradeRules.upgradeHelpText" | ||
| defaultMessage="Choose field values used in the upgraded rule. " | ||
| id="xpack.securitySolution.detectionEngine.rules.upgradeRules.comparisonSide.upgradeHelpText" | ||
| defaultMessage="The {title} lets you compare the values of a field across different versions of a rule: {versions} The differences are shown as JSON, highlighting additions, deletions, and modifications. Use this view to review and understand changes across versions." |
There was a problem hiding this comment.
Hi @nastasha-solomon! 😊 Could you kindly review the UI copy in this file? Please see PR description for screenshots. Thanks so much for your help!
|
Transitioning back to Draft as it turned out we may need to make a few more adjustments to UI. Will ping everyone once it's resolved. |
nikitaindik
changed the title
nikitaindik
force-pushed
the
better-popup-texts
branch
from
ab05a56 to
5a455a9
Compare
…ation-layout' into better-popup-texts
69 tasks
maximpn
reviewed
Dec 12, 2024
There was a problem hiding this comment.
@nikitaindik Thanks for radically simplifying Diff View comparison options 🙏
I've noticed My customizations option may confuse users and should be removed. I suggest to stick to 3 options My changes, Update from Elastic and My changes merged with Elastic's.
It looks like the implementation could be simplified by separating Diff options and an algorithm picking versions for comparison. We could stick to concrete versions but missing base version case requires using current version instead of base.
...agement/components/rule_details/three_way_diff/comparison_side/comparison_side_help_info.tsx
Outdated
Show resolved
Hide resolved
| ): Array<{ value: SelectedVersions; text: string; title: string }> => { | ||
| switch (fieldDiff.diff_outcome) { | ||
| case ThreeWayDiffOutcome.StockValueCanUpdate: { | ||
| const hasUserChangedResolvedValue = !isEqual(fieldDiff.merged_version, resolvedValue); |
There was a problem hiding this comment.
Calculating hasUserChangedResolvedValue here has maintenance issues. The knowledge that merged_version is used in finalDiffableRule is encapsulated in useFieldUpgradeContext. For example merged_version is replaced with something else in the future so this line have to be changed as well.
It looks cleaner to expose a value from useFieldUpgradeContext(). For example it could be named hasResolvedValue.
There was a problem hiding this comment.
Pushed the refactoring change. Please take a look.
...nagement/components/rule_details/three_way_diff/comparison_side/versions_picker/constants.ts
Outdated
Show resolved
Hide resolved
| @@ -7,6 +7,7 @@ | |||
|
|
|||
| export enum FieldUpgradeStateEnum { | |||
| NoUpdate = 'NO_UPDATE', | |||
| SameUpdate = 'SAME_UPDATE', | |||
There was a problem hiding this comment.
The word Same may be confusing. Maybe use MATCHING_UPDATE instead?
There was a problem hiding this comment.
Discussed this with Maxim yesterday, decided to stick consistent naming, since we use "same" in ThreeWayDiffOutcome (ThreeWayDiffOutcome.CustomizedValueSameUpdate).
...nagement/components/rule_details/three_way_diff/comparison_side/versions_picker/constants.ts
Outdated
Show resolved
Hide resolved
| ? FieldUpgradeStateEnum.NoConflict | ||
| : FieldUpgradeStateEnum.NoUpdate, | ||
| }; | ||
| if (fieldDiff.has_update) { |
There was a problem hiding this comment.
The logic becomes quite complex here. Do you think using fieldDiff.diff_outcome in switch instead of fieldDiff.conflict will make it more readable?
There was a problem hiding this comment.
Just tried refactoring it to use fieldDiff.diff_outcome in switch, but it actually became even more messy. Maybe we can take a look at it together?
maximpn
reviewed
Dec 12, 2024
...nagement/components/rule_details/three_way_diff/comparison_side/versions_picker/constants.ts
Show resolved
Hide resolved
…ldUpgradeContext`
|
@maximpn I've addressed the comments and pushed the changes. Please take a look. |
maximpn
approved these changes
Dec 13, 2024
There was a problem hiding this comment.
@nikitaindik Implementation looks much more readable with the recent changes 🎉 Thanks for incorporating my suggestions 🙏
I tested the PR and left comments as suggestions which are trivial to address.
...ngine/rule_management/components/rule_details/three_way_diff/comparison_side/translations.ts
Outdated
Show resolved
Hide resolved
...t/components/rule_details/three_way_diff/comparison_side/versions_picker/versions_picker.tsx
Outdated
Show resolved
Hide resolved
...t/components/rule_details/three_way_diff/comparison_side/versions_picker/versions_picker.tsx
Outdated
Show resolved
Hide resolved
...ule_management/components/rule_details/three_way_diff/rule_upgrade/field_upgrade_context.tsx
Show resolved
Hide resolved
...ction_engine/rule_management/components/rule_details/three_way_diff/comparison_side/utils.ts
Outdated
Show resolved
Hide resolved
...ction_engine/rule_management/components/rule_details/three_way_diff/comparison_side/utils.ts
Outdated
Show resolved
Hide resolved
|
Thank you, @maximpn! I've addressed your latest feedback and will merge it now. |
💚 Build SucceededMetrics [docs]Async chunks
History
cc @nikitaindik |
|
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/12329556495 |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Dec 14, 2024
… in Rule Upgrade flyout (elastic#203222) **Partially addresses: elastic#171520** ## Summary This PR updates the tooltips for the ‘Diff view’ and ‘Final update’ sections in the prebuilt rule upgrade flyout. It also streamlines the version picker by removing redundant options, making the UI simpler and clearer for users. ## Changes - Reduced the number of version picker items based on the diff outcome. Updated item names for better clarity. - Revised the tooltip text for the ‘Diff view’ section to better explain the available dropdown options. The tooltip now describes only the options in the dropdown to avoid overwhelming the user with unrelated information. - Updated the tooltip text for the ‘Final update’ section. ## Screenshots <img width="922" alt="Schermafbeelding 2024-12-11 om 11 54 48" src="https://github.com/user-attachments/assets/124e76a1-99dc-48d8-be54-f6c8f2079451"> <img width="640" alt="Schermafbeelding 2024-12-11 om 11 55 32" src="https://github.com/user-attachments/assets/45655dd2-6503-46b7-b28b-0df7bf0e6fa3"> <img width="433" alt="Schermafbeelding 2024-12-11 om 11 55 58" src="https://github.com/user-attachments/assets/d845ff52-4678-4245-8bdd-b9957f0c1d13"> Work started on 06-Dec-2024. --------- Co-authored-by: Maxim Palenov <maxim.palenov@elastic.co> (cherry picked from commit 90e35a0)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Dec 14, 2024
… texts in Rule Upgrade flyout (#203222) (#204314) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution] Reduce dropdown options and improve tooltip texts in Rule Upgrade flyout (#203222)](#203222) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Nikita Indik","email":"nikita.indik@elastic.co"},"sourceCommit":{"committedDate":"2024-12-14T11:40:45Z","message":"[Security Solution] Reduce dropdown options and improve tooltip texts in Rule Upgrade flyout (#203222)\n\n**Partially addresses: #171520**\r\n\r\n## Summary\r\n\r\nThis PR updates the tooltips for the ‘Diff view’ and ‘Final update’\r\nsections in the prebuilt rule upgrade flyout. It also streamlines the\r\nversion picker by removing redundant options, making the UI simpler and\r\nclearer for users.\r\n\r\n## Changes\r\n- Reduced the number of version picker items based on the diff outcome.\r\nUpdated item names for better clarity.\r\n- Revised the tooltip text for the ‘Diff view’ section to better explain\r\nthe available dropdown options. The tooltip now describes only the\r\noptions in the dropdown to avoid overwhelming the user with unrelated\r\ninformation.\r\n- Updated the tooltip text for the ‘Final update’ section.\r\n\r\n## Screenshots\r\n<img width=\"922\" alt=\"Schermafbeelding 2024-12-11 om 11 54 48\"\r\nsrc=\"https://github.com/user-attachments/assets/124e76a1-99dc-48d8-be54-f6c8f2079451\">\r\n\r\n<img width=\"640\" alt=\"Schermafbeelding 2024-12-11 om 11 55 32\"\r\nsrc=\"https://github.com/user-attachments/assets/45655dd2-6503-46b7-b28b-0df7bf0e6fa3\">\r\n\r\n<img width=\"433\" alt=\"Schermafbeelding 2024-12-11 om 11 55 58\"\r\nsrc=\"https://github.com/user-attachments/assets/d845ff52-4678-4245-8bdd-b9957f0c1d13\">\r\n\r\n\r\nWork started on 06-Dec-2024.\r\n\r\n---------\r\n\r\nCo-authored-by: Maxim Palenov <maxim.palenov@elastic.co>","sha":"90e35a04bc795aebdd62c11cd53e279b566582b8","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Detections and Resp","Team: SecuritySolution","Team:Detection Rule Management","Feature:Prebuilt Detection Rules","ci:cloud-deploy","backport:version","v8.18.0"],"title":"[Security Solution] Reduce dropdown options and improve tooltip texts in Rule Upgrade flyout","number":203222,"url":"https://github.com/elastic/kibana/pull/203222","mergeCommit":{"message":"[Security Solution] Reduce dropdown options and improve tooltip texts in Rule Upgrade flyout (#203222)\n\n**Partially addresses: #171520**\r\n\r\n## Summary\r\n\r\nThis PR updates the tooltips for the ‘Diff view’ and ‘Final update’\r\nsections in the prebuilt rule upgrade flyout. It also streamlines the\r\nversion picker by removing redundant options, making the UI simpler and\r\nclearer for users.\r\n\r\n## Changes\r\n- Reduced the number of version picker items based on the diff outcome.\r\nUpdated item names for better clarity.\r\n- Revised the tooltip text for the ‘Diff view’ section to better explain\r\nthe available dropdown options. The tooltip now describes only the\r\noptions in the dropdown to avoid overwhelming the user with unrelated\r\ninformation.\r\n- Updated the tooltip text for the ‘Final update’ section.\r\n\r\n## Screenshots\r\n<img width=\"922\" alt=\"Schermafbeelding 2024-12-11 om 11 54 48\"\r\nsrc=\"https://github.com/user-attachments/assets/124e76a1-99dc-48d8-be54-f6c8f2079451\">\r\n\r\n<img width=\"640\" alt=\"Schermafbeelding 2024-12-11 om 11 55 32\"\r\nsrc=\"https://github.com/user-attachments/assets/45655dd2-6503-46b7-b28b-0df7bf0e6fa3\">\r\n\r\n<img width=\"433\" alt=\"Schermafbeelding 2024-12-11 om 11 55 58\"\r\nsrc=\"https://github.com/user-attachments/assets/d845ff52-4678-4245-8bdd-b9957f0c1d13\">\r\n\r\n\r\nWork started on 06-Dec-2024.\r\n\r\n---------\r\n\r\nCo-authored-by: Maxim Palenov <maxim.palenov@elastic.co>","sha":"90e35a04bc795aebdd62c11cd53e279b566582b8"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/203222","number":203222,"mergeCommit":{"message":"[Security Solution] Reduce dropdown options and improve tooltip texts in Rule Upgrade flyout (#203222)\n\n**Partially addresses: #171520**\r\n\r\n## Summary\r\n\r\nThis PR updates the tooltips for the ‘Diff view’ and ‘Final update’\r\nsections in the prebuilt rule upgrade flyout. It also streamlines the\r\nversion picker by removing redundant options, making the UI simpler and\r\nclearer for users.\r\n\r\n## Changes\r\n- Reduced the number of version picker items based on the diff outcome.\r\nUpdated item names for better clarity.\r\n- Revised the tooltip text for the ‘Diff view’ section to better explain\r\nthe available dropdown options. The tooltip now describes only the\r\noptions in the dropdown to avoid overwhelming the user with unrelated\r\ninformation.\r\n- Updated the tooltip text for the ‘Final update’ section.\r\n\r\n## Screenshots\r\n<img width=\"922\" alt=\"Schermafbeelding 2024-12-11 om 11 54 48\"\r\nsrc=\"https://github.com/user-attachments/assets/124e76a1-99dc-48d8-be54-f6c8f2079451\">\r\n\r\n<img width=\"640\" alt=\"Schermafbeelding 2024-12-11 om 11 55 32\"\r\nsrc=\"https://github.com/user-attachments/assets/45655dd2-6503-46b7-b28b-0df7bf0e6fa3\">\r\n\r\n<img width=\"433\" alt=\"Schermafbeelding 2024-12-11 om 11 55 58\"\r\nsrc=\"https://github.com/user-attachments/assets/d845ff52-4678-4245-8bdd-b9957f0c1d13\">\r\n\r\n\r\nWork started on 06-Dec-2024.\r\n\r\n---------\r\n\r\nCo-authored-by: Maxim Palenov <maxim.palenov@elastic.co>","sha":"90e35a04bc795aebdd62c11cd53e279b566582b8"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Nikita Indik <nikita.indik@elastic.co>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Partially addresses: #171520
Summary
This PR updates the tooltips for the ‘Diff view’ and ‘Final update’ sections in the prebuilt rule upgrade flyout. It also streamlines the version picker by removing redundant options, making the UI simpler and clearer for users.
Changes
Screenshots
Work started on 06-Dec-2024.