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

[Security Solution] Fix app layout #76668

Merged
merged 25 commits into from
Sep 25, 2020
Merged

[Security Solution] Fix app layout #76668

merged 25 commits into from
Sep 25, 2020

Conversation

patrykkopycinski
Copy link
Contributor

@patrykkopycinski patrykkopycinski commented Sep 3, 2020 •
edited
Loading

Summary

Layout changes introduced in #74062, where we have moved scrolling from body inside the Security App cause side-effects, because EuiPopoover attaching event listeners to the window, so for example repositionOnScroll prop in popovers has stopped working properly.
This PR reverts scrolling to the body and using useThrottledResizeObserver to get the height of position:fixed HeaderGlobal to add padding-top to the Main component

image

Checklist

@patrykkopycinski patrykkopycinski self-assigned this Sep 3, 2020
@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream

@patrykkopycinski patrykkopycinski marked this pull request as ready for review September 7, 2020 09:37
@patrykkopycinski patrykkopycinski requested review from a team as code owners September 7, 2020 09:37
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@patrykkopycinski patrykkopycinski requested a review from a team as a code owner September 7, 2020 14:57
@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream

@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream

@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream

@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream

dmlemeshko
dmlemeshko approved these changes Sep 16, 2020
View reviewed changes
Copy link
Member

@dmlemeshko dmlemeshko left a comment

Choose a reason for hiding this comment

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

Kibana-QA changes LGTM

@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream

@andrew-goldstein
Copy link
Contributor

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@andrew-goldstein
Copy link
Contributor

Seeing a Safari-only issue on the Detections page in full screen mode, where the status bar is rendered past the bottom of the window, and the header scrolls with the page:

firefox-vs-safari

In the screenshot above, Firefox is on the left, and Safari is on the right, illustrating that the status bar is not visible in Safari.

To reproduce:

  1. Navigate to the Detections page
  2. Change the page size to 100
  3. Click the Full screen toggle

Expected results

  • The status bar / paging controls are visible at the bottom of the page
  • The header containing the Showing n alerts and Open, In progress, and Closed buttons remain in a fixed position when the view is scrolled

Actual results

  • In Safari 14.0, the status bar / paging controls are not visible at the bottom of the page
  • In Safari 14.0, when the (full screen) view is vertically scrolled, the header containing the Showing n alerts and Open, In progress, and Closed buttons scrolls

@andrew-goldstein
Copy link
Contributor

Seeing a Safari-only issue on the Detections page where Resolver isn't expanding to take up the available space in full screen mode on the Detections page, per the screenshot below:

resolver-full-screen

To reproduce:

  1. Navigate to the Detections page

  2. Enter the following KQL: agent.type: endpoint and process.entity_id: *

  3. Click Analyze event

  4. Click the Full screen toggle button

Expected result:

  • Resolver expands to fill the available space

Actual result

  • Resolver contracts to a size smaller than it was before switching to Full screen mode instead of expanding to take up the available space

andrew-goldstein
andrew-goldstein approved these changes Sep 24, 2020
View reviewed changes
Copy link
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

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

Thanks for these fixes @patrykkopycinski! 🙏

I desk tested regular and full screen modes throughout the app on both a large monitor and a laptop in the following browsers:

  • Chrome 85.0.4183.121
  • Firefox 80.0.1
  • Safari 14.0

LGTM 🚀

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id value diff baseline
securitySolution 10.2MB +974.0B 10.2MB

page load bundle size

id value diff baseline
securitySolution 810.4KB +120.0B 810.3KB

History

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

@patrykkopycinski patrykkopycinski merged commit 012fa42 into elastic:master Sep 25, 2020
@patrykkopycinski patrykkopycinski deleted the fix/siem-app-fixed-header-layout branch September 25, 2020 12:15
@patrykkopycinski patrykkopycinski mentioned this pull request Sep 25, 2020
Merged
patrykkopycinski added a commit to patrykkopycinski/kibana that referenced this pull request Sep 25, 2020
@patrykkopycinski
[Security Solution] Fix app layout (elastic#76668)
324eaf3
patrykkopycinski added a commit that referenced this pull request Sep 25, 2020
@patrykkopycinski
[Security Solution] Fix app layout (#76668) (#78532)
6d0257a
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 28, 2020
@gmmorris
Merge branch 'master' into task-manager/health
92cf5ad 
* master: (226 commits)
  [Enterprise Search] Added Logic for the Credentials View (elastic#77626)
  [CSM] Js errors (elastic#77919)
  Add the @kbn/apm-config-loader package (elastic#77855)
  [Security Solution] Refactor useSelector (elastic#75297)
  Implement tagcloud renderer (elastic#77910)
  [APM] Alerting: Add global option to create all alert types (elastic#78151)
  [Ingest pipelines] Upload indexed document to test a pipeline (elastic#77939)
  TypeScript cleanup in visualizations plugin (elastic#78428)
  Lazy load metric & mardown visualizations (elastic#78391)
  [Detections][EQL] EQL rule execution in detection engine (elastic#77419)
  Update tutorial-full-experience.asciidoc (elastic#75836)
  Update tutorial-define-index.asciidoc (elastic#75754)
  Add support for runtime field types to mappings editor. (elastic#77420)
  [Monitoring] Usage collection (elastic#75878)
  [Docs][Actions] Add docs for Jira and IBM Resilient (elastic#78316)
  [Security Solution][Resolver] Update @timestamp formatting (elastic#78166)
  [Security Solution] Fix app layout (elastic#76668)
  [Security Solution][Resolver] 2 new functions to DAL (elastic#78477)
  Adds new elasticsearch client to telemetry plugin (elastic#78046)
  skip flaky suite (elastic#78512) (elastic#78511) (elastic#78510) (elastic#78509) (elastic#78508) (elastic#78507) (elastic#78506) (elastic#78505) (elastic#78504) (elastic#78503) (elastic#78502) (elastic#78501) (elastic#78500)
  ...
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmlemeshko dmlemeshko dmlemeshko approved these changes

@andrew-goldstein andrew-goldstein andrew-goldstein approved these changes

Assignees

@patrykkopycinski patrykkopycinski

Labels
release_note:fix Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@patrykkopycinski @elasticmachine @andrew-goldstein @kibanamachine @dmlemeshko @MindyRS @MadameSheema @XavierM