[Expandable Flyout] - customize default right, left and preview widths for push mode#206155
Conversation
There was a problem hiding this comment.
desk tested and it was very smooth!
@PhilippeOberti Will there be separation for the user specified widths as well? I noticed once I adjust a section, push and overlay then use the same width (as expected).
I also left some comments about readability and aims to reduce duplication, let me know what you think!
x-pack/solutions/security/packages/expandable-flyout/src/store/reducers.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
consider grouping them into 2 objects for better readability
const Overlay = {
RIGHT_SECTION_MIN_WIDTH = ...
RIGHT_SECTION_Max_WIDTH...
....
}
There was a problem hiding this comment.
yup good idea! I went back and worth a ton on how to group them. It's way more complex that I though as some of these values are used in both push and overlay mode, for right and left sections...
I ended up refactoring this file quite a bit. I extracted some of the logic in 2 functions, which I think is a lot more clear as I could document them in details.
Let me know what you think of the new code!
There was a problem hiding this comment.
could you add some comments about BIG_RESOLUTION_BREAKPOINT, FULL_WIDTH_BREAKPOINT, and their difference from MIN_RESOLUTION_BREAKPOINT and MAX_RESOLUTION_BREAKPOINT?
looks like the latter 2 are used exclusively in overlay mode, maybe adding an overlay reference? or put it in the overlay const object if you take the grouping suggestion
There was a problem hiding this comment.
same comment as I made above, let me know what you think of the new code!
x-pack/solutions/security/packages/expandable-flyout/src/hooks/use_window_width.ts
Outdated
Show resolved
Hide resolved
PhilippeOberti
force-pushed
the
expandable-flyout-default-push-widths
branch
from
975af82 to
6295a8e
Compare
There was a problem hiding this comment.
what do you think about referencing the name of the number (Right panel min) versus actual number? if the number is changed in the future, the dev needs to remember to change all the comments
or vice versa - use numbers in the function directly and matches the comments?
There was a problem hiding this comment.
so I went back and forth on this many times while I was developing. I really did not like using the name of the constant as it makes the documentation extremely hard to read. For me documentation is for devs who want to quickly understand what's happening, so it should be humanly readable, so I like the actual values there.
I understand that updating the values in the code would require an update in the documentation, but I feel that is valid for pretty much any documentation. If you have props or logic change of component or hook, you'll have to manually update the doc accordingly. Here's it's a bit more involved I agree, but I feel like changing these values should not happen too many times (hopefully never 😆)
I did also consider using the values directly in the code (and not have any constants) but I feel like this would go against the patterns of having constants defined at all... That's why I used those constants, but also added code comments directly above their usage to indicate what's happening in a readable way...
This is definitely not a science, it was just what I felt was the most readable...
I did update some more comments to use the values directly instead of the names of the constants. Let me know if you feel strongly against that approach, I'm happy to discuss this with the rest of team to see what everyone else prefers!
Thanks 😄
There was a problem hiding this comment.
@PhilippeOberti I think the revised comments really helped! this is not a section we expect a lot of changes (famous last words lol), so having comprehensive comments, and being able to match the numbers to the constants above are sufficient.
x-pack/solutions/security/packages/expandable-flyout/src/store/reducers.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/security/packages/expandable-flyout/src/store/reducers.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/security/packages/expandable-flyout/src/store/reducers.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/security/packages/expandable-flyout/src/hooks/use_window_width.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/security/packages/expandable-flyout/src/hooks/use_window_width.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/security/packages/expandable-flyout/src/hooks/use_window_width.ts
Outdated
Show resolved
Hide resolved
x-pack/solutions/security/packages/expandable-flyout/src/hooks/use_window_width.ts
Outdated
Show resolved
Hide resolved
PhilippeOberti
force-pushed
the
expandable-flyout-default-push-widths
branch
from
6295a8e to
64e04f2
Compare
PhilippeOberti
added
release_note:skip
|
Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations) |
There was a problem hiding this comment.
@PhilippeOberti I think the revised comments really helped! this is not a section we expect a lot of changes (famous last words lol), so having comprehensive comments, and being able to match the numbers to the constants above are sufficient.
There was a problem hiding this comment.
nit: what do you think about matching LEFT and RIGHT naming conventions?
There was a problem hiding this comment.
good point, I'm not sure how I missed that, now that I'm seeing it, it bothers me so much 😆
I will improve before merging!
PhilippeOberti
force-pushed
the
expandable-flyout-default-push-widths
branch
from
bfcc166 to
7030902
Compare
💔 Build Failed
Failed CI Steps
Metrics [docs]Async chunks
History
|
|
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/13188639909 |
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
…s for push mode (elastic#206155) ## Summary This PR is making some changes to the Expandable Flyout package. Prior work had added [push mode](elastic#182615) to the package, added [custom way](elastic#170078) to handle the width for multiple resolutions, and [added support](elastic#192906) for the internal section to be resiable by users. This PR improves the default user experience when using the flyout in push mode. Until now, the default `right`, `left` and `preview` width in `push` mode and `overlay` mode were identical. This meant that the flyout rendered in `push` mode was most of the time using the whole screen, not leaving any room to the rest of the page content (like the alerts table). The `push` widths are now calculated in a different way, to leave as much room as possible while still allowing the flyout `right` and `left` sections to render their content correctly, at least most of the time. Users can still resize the whole flyout as well as the internal `right` and `left` sections. The `push` widths are generally smaller/narrower than the `overlay` widths. #### The `overlay` mode default widths have not changed https://github.com/user-attachments/assets/28b6c41e-b12c-45cf-aa3e-026a7acdb7b3 #### The `push` mode default widths https://github.com/user-attachments/assets/93706f9e-212b-4cb4-8748-552f2daed585 ### Checklist - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios (cherry picked from commit e7140ff) # Conflicts: # x-pack/solutions/security/packages/expandable-flyout/src/components/preview_section.tsx
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…s for push mode (elastic#206155) ## Summary This PR is making some changes to the Expandable Flyout package. Prior work had added [push mode](elastic#182615) to the package, added [custom way](elastic#170078) to handle the width for multiple resolutions, and [added support](elastic#192906) for the internal section to be resiable by users. This PR improves the default user experience when using the flyout in push mode. Until now, the default `right`, `left` and `preview` width in `push` mode and `overlay` mode were identical. This meant that the flyout rendered in `push` mode was most of the time using the whole screen, not leaving any room to the rest of the page content (like the alerts table). The `push` widths are now calculated in a different way, to leave as much room as possible while still allowing the flyout `right` and `left` sections to render their content correctly, at least most of the time. Users can still resize the whole flyout as well as the internal `right` and `left` sections. The `push` widths are generally smaller/narrower than the `overlay` widths. #### The `overlay` mode default widths have not changed https://github.com/user-attachments/assets/28b6c41e-b12c-45cf-aa3e-026a7acdb7b3 #### The `push` mode default widths https://github.com/user-attachments/assets/93706f9e-212b-4cb4-8748-552f2daed585 ### Checklist - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
… widths for push mode (#206155) (#210117) # Backport This will backport the following commits from `main` to `8.x`: - [[Expandable Flyout] - customize default right, left and preview widths for push mode (#206155)](#206155) <!--- Backport version: 9.6.4 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Philippe Oberti","email":"philippe.oberti@elastic.co"},"sourceCommit":{"committedDate":"2025-02-06T21:57:07Z","message":"[Expandable Flyout] - customize default right, left and preview widths for push mode (#206155)\n\n## Summary\r\n\r\nThis PR is making some changes to the Expandable Flyout package. Prior\r\nwork had added [push\r\nmode](#182615) to the package,\r\nadded [custom way](#170078) to\r\nhandle the width for multiple resolutions, and [added\r\nsupport](#192906) for the internal\r\nsection to be resiable by users.\r\n\r\nThis PR improves the default user experience when using the flyout in\r\npush mode. Until now, the default `right`, `left` and `preview` width in\r\n`push` mode and `overlay` mode were identical. This meant that the\r\nflyout rendered in `push` mode was most of the time using the whole\r\nscreen, not leaving any room to the rest of the page content (like the\r\nalerts table).\r\n\r\nThe `push` widths are now calculated in a different way, to leave as\r\nmuch room as possible while still allowing the flyout `right` and `left`\r\nsections to render their content correctly, at least most of the time.\r\nUsers can still resize the whole flyout as well as the internal `right`\r\nand `left` sections. The `push` widths are generally smaller/narrower\r\nthan the `overlay` widths.\r\n\r\n#### The `overlay` mode default widths have not changed\r\n\r\n\r\nhttps://github.com/user-attachments/assets/28b6c41e-b12c-45cf-aa3e-026a7acdb7b3\r\n\r\n#### The `push` mode default widths\r\n\r\n\r\nhttps://github.com/user-attachments/assets/93706f9e-212b-4cb4-8748-552f2daed585\r\n\r\n### Checklist\r\n\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"e7140ff25fbf63f20b930da44aaebb5811b6d1ba","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Threat Hunting:Investigations","backport:version","v9.1.0","v8.19.0"],"title":"[Expandable Flyout] - customize default right, left and preview widths for push mode","number":206155,"url":"https://github.com/elastic/kibana/pull/206155","mergeCommit":{"message":"[Expandable Flyout] - customize default right, left and preview widths for push mode (#206155)\n\n## Summary\r\n\r\nThis PR is making some changes to the Expandable Flyout package. Prior\r\nwork had added [push\r\nmode](#182615) to the package,\r\nadded [custom way](#170078) to\r\nhandle the width for multiple resolutions, and [added\r\nsupport](#192906) for the internal\r\nsection to be resiable by users.\r\n\r\nThis PR improves the default user experience when using the flyout in\r\npush mode. Until now, the default `right`, `left` and `preview` width in\r\n`push` mode and `overlay` mode were identical. This meant that the\r\nflyout rendered in `push` mode was most of the time using the whole\r\nscreen, not leaving any room to the rest of the page content (like the\r\nalerts table).\r\n\r\nThe `push` widths are now calculated in a different way, to leave as\r\nmuch room as possible while still allowing the flyout `right` and `left`\r\nsections to render their content correctly, at least most of the time.\r\nUsers can still resize the whole flyout as well as the internal `right`\r\nand `left` sections. The `push` widths are generally smaller/narrower\r\nthan the `overlay` widths.\r\n\r\n#### The `overlay` mode default widths have not changed\r\n\r\n\r\nhttps://github.com/user-attachments/assets/28b6c41e-b12c-45cf-aa3e-026a7acdb7b3\r\n\r\n#### The `push` mode default widths\r\n\r\n\r\nhttps://github.com/user-attachments/assets/93706f9e-212b-4cb4-8748-552f2daed585\r\n\r\n### Checklist\r\n\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"e7140ff25fbf63f20b930da44aaebb5811b6d1ba"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/206155","number":206155,"mergeCommit":{"message":"[Expandable Flyout] - customize default right, left and preview widths for push mode (#206155)\n\n## Summary\r\n\r\nThis PR is making some changes to the Expandable Flyout package. Prior\r\nwork had added [push\r\nmode](#182615) to the package,\r\nadded [custom way](#170078) to\r\nhandle the width for multiple resolutions, and [added\r\nsupport](#192906) for the internal\r\nsection to be resiable by users.\r\n\r\nThis PR improves the default user experience when using the flyout in\r\npush mode. Until now, the default `right`, `left` and `preview` width in\r\n`push` mode and `overlay` mode were identical. This meant that the\r\nflyout rendered in `push` mode was most of the time using the whole\r\nscreen, not leaving any room to the rest of the page content (like the\r\nalerts table).\r\n\r\nThe `push` widths are now calculated in a different way, to leave as\r\nmuch room as possible while still allowing the flyout `right` and `left`\r\nsections to render their content correctly, at least most of the time.\r\nUsers can still resize the whole flyout as well as the internal `right`\r\nand `left` sections. The `push` widths are generally smaller/narrower\r\nthan the `overlay` widths.\r\n\r\n#### The `overlay` mode default widths have not changed\r\n\r\n\r\nhttps://github.com/user-attachments/assets/28b6c41e-b12c-45cf-aa3e-026a7acdb7b3\r\n\r\n#### The `push` mode default widths\r\n\r\n\r\nhttps://github.com/user-attachments/assets/93706f9e-212b-4cb4-8748-552f2daed585\r\n\r\n### Checklist\r\n\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials\r\n- [ ] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios","sha":"e7140ff25fbf63f20b930da44aaebb5811b6d1ba"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->
4 participants
Summary
This PR is making some changes to the Expandable Flyout package. Prior work had added push mode to the package, added custom way to handle the width for multiple resolutions, and added support for the internal section to be resiable by users.
This PR improves the default user experience when using the flyout in push mode. Until now, the default
right,leftandpreviewwidth inpushmode andoverlaymode were identical. This meant that the flyout rendered inpushmode was most of the time using the whole screen, not leaving any room to the rest of the page content (like the alerts table).The
pushwidths are now calculated in a different way, to leave as much room as possible while still allowing the flyoutrightandleftsections to render their content correctly, at least most of the time. Users can still resize the whole flyout as well as the internalrightandleftsections. Thepushwidths are generally smaller/narrower than theoverlaywidths.The
overlaymode default widths have not changedScreen.Recording.2025-01-09.at.6.21.29.PM.mov
The
pushmode default widthsScreen.Recording.2025-01-09.at.6.20.27.PM.mov
Checklist