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

[SIEM] Table Styles & Markup Tweaks #46300

Merged
merged 78 commits into from
Oct 2, 2019
Merged

[SIEM] Table Styles & Markup Tweaks #46300

merged 78 commits into from
Oct 2, 2019

Conversation

MichaelMarcialis
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis commented Sep 20, 2019 •
edited
Loading

Summary

This PR makes a number of markup and style changes to the in-page and timeline tables for general quality-of-life and better integration with the updated draggables styles.

My apologies for the size of this PR. I went in expecting to make a few minor tweaks, but the changes I wanted to make necessitated other changes be made to support them.

A big thanks to @andrew-goldstein and @FrankHassanabad for helping me out with my questions <3

Issue: #45429

Changes

All SIEM Tables

  • General cleanup of column header text and column widths.
  • Refactor of TruncatableText component to allow for dynamic width truncation, rather than having to pass a static width prop.
  • Address a handful of IE11 bugs that I encountered.

Network Page Source/Destination IP Tables

  • Changing out responsive EuiFlexGroup via matchMedia to a simpler CSS-based media query (this was necessary for IE11).

Timelines Page Table

  • Utilize compressed prop for consistency with other tables across SIEM.

Before
image

After
image

Timeline Events Table

  • Refactor of table structural HTML and styles to play nicer with the latest draggables changes, reduce some complexity, and prepare for further future enhancements.
  • Correct truncation and tooltip bugs.
  • Merge the previous vertical and horizontal scroll containers into a single scrolling container, which corrects the issue of the vertical scrollbar being hidden off-screen, to the right.
  • Apply ARIA landmark roles for accessibility (as this table is div-based and not a traditional HTML table.
  • Localize sorting onClick to a new column header button for easier identifiability and better accessibility.
  • Update row visibility placeholder styles to appear more like a skeleton screen while the data is being loaded (thanks to @FrankHassanabad for the idea; we can continue to enhance in the future).
  • Indentation of event supplement data (notes, attributes, row renderers) to better show the hierarchy of information for each event row.
  • General touchup of column resize handles styles and localizing their usage to only the table header area. I feel this is a more common user experience, will match with the experience of the upcoming datagrid EUI component, and can potentially prevent user mis-clicks in the body.

Before
image

After
image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@MichaelMarcialis
restore conditional space before AS number
5df9bf6
@MichaelMarcialis
touchup table widths and text
ae46563
@MichaelMarcialis
adjust datepicker width
ef9f350
@MichaelMarcialis
refactor matchMedia; set bp to above mbp rez
50a8a1c
@MichaelMarcialis
timeline table body refactor, first pass
51ded2a
@MichaelMarcialis
TruncatableText: rm “width”, added “truncated”
0b1587a
@MichaelMarcialis
cleanup imports
17fa005
@MichaelMarcialis
cleanup styles
d6281b6
@MichaelMarcialis
rm size prop
5b908c5
@MichaelMarcialis
swap out div? prob need to fix ref
2c08e03
@MichaelMarcialis
Merge branch 'master' of https://github.com/elastic/kibana into table…
22bd360 
…-cleanup

# Conflicts:
#	x-pack/legacy/plugins/siem/public/components/drag_and_drop/draggable_wrapper.tsx
#	x-pack/legacy/plugins/siem/public/components/fields_browser/field_items.tsx
#	x-pack/legacy/plugins/siem/public/components/fields_browser/field_name.tsx
#	x-pack/legacy/plugins/siem/public/components/formatted_ip/index.tsx
#	x-pack/legacy/plugins/siem/public/components/pin/index.tsx
#	x-pack/legacy/plugins/siem/public/components/timeline/body/actions/index.tsx
#	x-pack/legacy/plugins/siem/public/components/timeline/body/column_headers/header/index.tsx
#	x-pack/legacy/plugins/siem/public/components/timeline/body/data_driven_columns/index.tsx
#	x-pack/legacy/plugins/siem/public/components/timeline/body/events/event_column_view.tsx
#	x-pack/legacy/plugins/siem/public/components/timeline/body/events/stateful_event.tsx
#	x-pack/legacy/plugins/siem/public/components/timeline/body/renderers/column_renderer.ts
#	x-pack/legacy/plugins/siem/public/components/timeline/body/renderers/empty_column_renderer.tsx
#	x-pack/legacy/plugins/siem/public/components/timeline/body/renderers/formatted_field.tsx
#	x-pack/legacy/plugins/siem/public/components/timeline/body/renderers/helpers.tsx
#	x-pack/legacy/plugins/siem/public/components/timeline/body/renderers/plain_column_renderer.tsx
#	x-pack/legacy/plugins/siem/public/components/timeline/properties/helpers.tsx
#	x-pack/legacy/plugins/siem/public/pages/network/network.tsx
@MichaelMarcialis
restore truncation in timeline
2098355
@MichaelMarcialis
think i have text overflow and tooltips happy now
53fc5f5
@MichaelMarcialis
light clean up
1a8d729
@MichaelMarcialis
single overflow scrolling element
0b1b3a0
@MichaelMarcialis
use polished for hex in rgba needs
7b49788
@MichaelMarcialis
simplify body markup
16ba724
@MichaelMarcialis
events table header poc
f3a00b9
@MichaelMarcialis
Merge branch 'master' of https://github.com/elastic/kibana into table…
41deb63 
…-cleanup

# Conflicts:
#	x-pack/legacy/plugins/siem/public/components/fields_browser/category_columns.tsx
#	x-pack/legacy/plugins/siem/public/components/fields_browser/field_name.tsx
#	x-pack/legacy/plugins/siem/public/pages/network/network.tsx
@MichaelMarcialis
close button fixes
2319781
@MichaelMarcialis
improve sort indicator position
84fc6a9
@MichaelMarcialis
drag handle updates
b5dd8b6
@MichaelMarcialis
fix fields browser positioning
6a64a8a
@MichaelMarcialis
apply aria roles
373f549
@MichaelMarcialis
fix blown out table width
64fa92d
@MichaelMarcialis
localize sorting onClick to header text
02b90f6
@MichaelMarcialis
correct key placement
10125f3
@MichaelMarcialis
prevent hover and click for unsortable and add btn
25ea213
@MichaelMarcialis
Merge branch 'master' of https://github.com/elastic/kibana into table…
b76330f 
…-cleanup
@MichaelMarcialis
rm btn for non aggregatable col headers
a0ccbd1
@MichaelMarcialis
Copy link
Contributor Author

Seeing an issue where column headers are sometimes not draggable.

@andrew-goldstein: Oooh, good catch! It looks like the dragging doesn't fire when the origin of the click starts at the new button element I put for the sorting of the column headers. Odd that it seems OK with a elements though. I'll have a look and see about fixing. Thanks!

@andrew-goldstein
Copy link
Contributor

The (much appreciated) improvements to scrolling in this PR don't yet interact with the JS that disables scrolling during a drag. As a result, this creates a subtle new bug where it's not possible to drag a field from an expanded event to the header when the timeline is scrolled. This also effects D&D from the Fields browser when the same conditions occur.

To reproduce:

  1. Click the SIEM icon in the left-hand navigation of Kibana to force a full page refresh (to clear any state)
  2. Drag a host to the timeline
  3. Scroll to the bottom of the timeline
  4. Expand an event
  5. Attempt to drag and drop a field, e.g. cloud.instance.id from the expanded event into the timeline column headers, to add the field in the desired location

Expected result:

  • The field is added to the timeline in the desired location

Actual result:

  • The timeline scrolls, and the field is NOT added to the timeline
  1. Once again, scroll to the bottom of the timeline
  2. Click the Columns button to open the Fields Browser
  3. Click on the agent category
  4. Attempt to drag a field, e.g. agent.id to the header of the timeline, to add the field in the desired location

Expected result:

  • The field is added to the timeline

Actual result:

  • The timeline scrolls behind the Fields Browser, and the field is NOT added to the timeline whe the field is dropped

I recommend merging this PR as-is, and addressing this issue in bouncing effect when dragging from condensed table formats. I'll add note to the issue above referencing this PR.

@andrew-goldstein
Copy link
Contributor

The column headers in the timeline and events viewer aren't sticky in Safari anymore. To reproduce:

  1. Click the SIEM icon in the left-hand navigation of Kibana to force a full page refresh (to clear any state)
  2. Drag a host to the timeline
  3. Scroll to the bottom of the timeline

Expected result:

  • The timeline header remains sticky

Actual result:

  • The timeline header scrolls out of view

(similar behavior exists in the Events tab on the Hosts page)

@andrew-goldstein
Copy link
Contributor

Seeing another new Safari-specific issue where the Fields Browser jumps around when a category is selected. It's more easily reproduced when there's more content rendered in the timeline:

To reproduce:

  1. Click the SIEM icon in the left-hand navigation of Kibana to force a full page refresh (to clear any state)
  2. Navigate to the Hosts page
  3. Drag the siem-kibana host from the All hosts widget to the timeline
  4. Click the Columns button to open the Fields Browser
  5. Click the elasticsearch category

Expected result:

  • The Fields Browser maintains its position on the page

Actual result:

  • The position of the Fields Browser shifts up, cutting off the title, search bar, etc

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@MichaelMarcialis
Copy link
Contributor Author

The column headers in the timeline and events viewer aren't sticky in Safari anymore.

@andrew-goldstein: Another great catch! In investigating further, it appears that Safari has a bug with sticky where it won't work properly with a container of a defined height and overflow: auto;. Looks like Safari is the new IE6 these days :/

Will investigate further for a fix.

@andrew-goldstein
Copy link
Contributor

Looks like Safari is the new IE6 these days :/

To that end, thanks @MichaelMarcialis for all the effort in this PR to make the timeline work with IE11!

@MichaelMarcialis
Copy link
Contributor Author

  • The position of the Fields Browser shifts up, cutting off the title, search bar, etc

@andrew-goldstein: Interesting; I don't think I'm seeing this bug on my end. I may have inadvertently fixed it when I was making the other Safari sticky column headers fix. Would you mind pulling in my latest changes and giving it another try on your end?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@andrew-goldstein
Copy link
Contributor

  • The position of the Fields Browser shifts up, cutting off the title, search bar, etc

@andrew-goldstein: Interesting; I don't think I'm seeing this bug on my end. I may have inadvertently fixed it when I was making the other Safari sticky column headers fix. Would you mind pulling in my latest changes and giving it another try on your end?

Yes, the fix for sticky headers in Safari also fixes the shifting! 😄

I think that last change had a small impact on the position of the Fields Browser, as it's now slightly overlapping the headers:

fields-browser-position

looking good overall!

@MichaelMarcialis
Copy link
Contributor Author

I think that last change had a small impact on the position of the Fields Browser, as it's now slightly overlapping the headers

Cool. I'll fix the overlap in my next commit. Otherwise I think that addresses all of your actionable comments.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

andrew-goldstein
andrew-goldstein approved these changes Oct 1, 2019
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.

There are a lot of appreciated improvements in this PR, big and small, including the scrolling enhancements to timeline, a11y, and IE11 support to name a few! ❤️ Tested locally in both dark and light modes locally in Chrome, FF, Safari, and (a bit, for the first time) in IE11!

LGTM 🙏 🚀

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@MichaelMarcialis MichaelMarcialis merged commit 58c1f87 into elastic:master Oct 2, 2019
@MichaelMarcialis MichaelMarcialis deleted the table-cleanup branch October 2, 2019 15:06
@MichaelMarcialis MichaelMarcialis mentioned this pull request Oct 2, 2019
Merged
MichaelMarcialis added a commit that referenced this pull request Oct 2, 2019
@MichaelMarcialis
[SIEM] Table Styles & Markup Tweaks (#46300) (#47131)
d2cddc9 
* restore conditional space before AS number

* touchup table widths and text

* adjust datepicker width

* refactor matchMedia; set bp to above mbp rez

* timeline table body refactor, first pass

* TruncatableText: rm “width”, added “truncated”

* cleanup imports

* cleanup styles

* rm size prop

* swap out div? prob need to fix ref

* restore truncation in timeline

* think i have text overflow and tooltips happy now

* light clean up

* single overflow scrolling element

* use polished for hex in rgba needs

* simplify body markup

* events table header poc

* close button fixes

* improve sort indicator position

* drag handle updates

* fix fields browser positioning

* apply aria roles

* fix blown out table width

* localize sorting onClick to header text

* correct key placement

* prevent hover and click for unsortable and add btn

* rm btn for non aggregatable col headers

* change width/height prop names to avoid html attr

* fix loading alignment

* account for action cell width when one action

* clean up trGroup organization

* imports cleanup

* fix types and skeleton rows/cells poc

* new skeleton row comp

* fix column heads not dragging

* supplement row indentation

* move widths out of styled components for perf

* inline dynamic width

* account for inline styles with dnd

* cleanup

* tweak in-page events table styling for consistency

* cleanup

* make compressed for consistency

* cleanup

* update jest tests + change matchMedia to css mq

* fix missing grab cursor in IE11

* fix action td group width in IE11

* fix columns menu positioning in IE11

* fix collapsing notes in timelines table in IE11

* decouple from DroppableWrapper to prevent issues

* update snapshots

* more specific selector

* rm show prop

* add truncate to shouldComponentUpdate

* bulk up `HeaderPanel` unit tests

* correct conditional styles and add some more tests

* improve SkeletonRow unit tests

* change for loop to map

* switch from pure to React.memo

* make SkeletonRow cellCount dynamic

* rm comments

* fix buttons not being draggable for col headers

* fix for safari position sticky + overflow auto bug

* correct type errors

* correct field browser overlap

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

@stephmilovic stephmilovic stephmilovic left review comments

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

@FrankHassanabad FrankHassanabad Awaiting requested review from FrankHassanabad

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:SIEM v7.5.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MichaelMarcialis @elasticmachine @stephmilovic @andrew-goldstein