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

[RAC] Add loading and empty states to the alerts table - Take II #110504

Merged
merged 24 commits into from
Sep 3, 2021
Merged

[RAC] Add loading and empty states to the alerts table - Take II #110504

merged 24 commits into from
Sep 3, 2021

Conversation

afgomez
Copy link
Contributor

@afgomez afgomez commented Aug 30, 2021 •
edited
Loading

Summary

Closes #104923.
Closes #109375.
Closes #108432.

Implements the loading and empty states in the alerts table according to design

Screenshots

Observability

Screenshot 2021-08-24 at 19 11 47

Screenshot 2021-08-24 at 19 11 11

Security

Screenshot 2021-09-01 at 15 54 48

Screenshot 2021-08-30 at 18 30 13

Checklist

Delete any items that are not applicable to this PR.

Alejandro Fernández Gómez added 7 commits August 30, 2021 17:44
@afgomez afgomez added v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed Feature:RAC label obsolete v7.16.0 Feature:Observability RAC labels Aug 30, 2021
@afgomez afgomez mentioned this pull request Aug 30, 2021
Closed
2 tasks
@afgomez afgomez marked this pull request as ready for review August 30, 2021 16:40
@afgomez afgomez requested a review from a team as a code owner August 30, 2021 16:40
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@afgomez afgomez requested a review from a team August 30, 2021 16:41
@weltenwort weltenwort self-requested a review August 30, 2021 17:30
weltenwort
weltenwort approved these changes Aug 30, 2021
View reviewed changes
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

This looks good on the observability alerts page, thank you. 👍

I couldn't get the security alerts page to show anything, so I couldn't check that.

I left a few stylistic nitpicks below. 😉

x-pack/plugins/timelines/public/components/t_grid/styles.tsx Outdated Show resolved Hide resolved
x-pack/plugins/timelines/public/components/t_grid/standalone/index.tsx Outdated Show resolved Hide resolved
x-pack/plugins/timelines/public/components/t_grid/standalone/index.tsx Show resolved Hide resolved
x-pack/plugins/timelines/public/components/t_grid/empty/index.tsx Outdated Show resolved Hide resolved
x-pack/plugins/timelines/public/components/t_grid/integrated/index.tsx Outdated Show resolved Hide resolved
x-pack/plugins/timelines/public/components/t_grid/empty/index.tsx Outdated Show resolved Hide resolved
@afgomez
Copy link
Contributor Author

afgomez commented Sep 1, 2021

@elasticmachine merge upstream

@afgomez
Copy link
Contributor Author

afgomez commented Sep 1, 2021

@machadoum thanks for the review!

Regarding the first point, I think we can keep only the spinner.

Good catch with the filters! I think it's worth then leaving them in the UI even if they make it a bit dirtier. @mdefazio what do you think?

@afgomez
Copy link
Contributor Author

afgomez commented Sep 1, 2021

@elasticmachine merge upstream

@kibanamachine kibanamachine requested a review from a team as a code owner September 1, 2021 15:47
@mdefazio
Copy link
Contributor

mdefazio commented Sep 2, 2021

Additional filters are removed when the empty state shows up.

@machadoum Thanks for pointing this out. It looks like they also disappear just when the data is trying to load (when EuiProgress is shown on panel). I agree these should remain with the other DataGrid controls.

Am I correct in seeing the following loading states within the DataGrid?:

  • Component load (EuiLoadingContent)
  • Data loading into DataGrid (EuiSpinner)
  • DataGrid is empty until rows within view area displayed (is this the data load portion?)
  • Change additional filters (EuiProgress on panel) --> This also happens when adding new fields to the DataGrid.

From my understanding, Security uses EuiLoadingContent most places when loading a table/panel content, so while I would prefer the spinner, it would seem we would need to then change this everywhere (which I'm not sure we want to atm)?

@machadoum machadoum requested a review from mdefazio September 2, 2021 12:21
machadoum
machadoum approved these changes Sep 2, 2021
View reviewed changes
Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

I tested it and it works fine. Approving it!

@mdefazio You are right, we do use EuiLoadingContent for all tables on the Security Solution side.
Screenshot 2021-09-02 at 14 19 07

Take a look at how it works after the last changes. What do you think?
Sep-02-2021 14-18-13

@afgomez
Copy link
Contributor Author

afgomez commented Sep 2, 2021

@elasticmachine merge upstream

@afgomez afgomez added the v7.15.0 label Sep 3, 2021
@afgomez afgomez enabled auto-merge (squash) September 3, 2021 09:25
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
timelines 329 330 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 6.5MB 6.5MB -281.0B
timelines 422.2KB 421.0KB -1.2KB
total -1.5KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
timelines 308.3KB 311.5KB +3.2KB

History

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

@afgomez afgomez merged commit 4e9e7a8 into elastic:master Sep 3, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Sep 3, 2021
@kibanamachine
[RAC] Add loading and empty states to the alerts table - Take II (ela…
d252468 
…stic#110504)

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine kibanamachine mentioned this pull request Sep 3, 2021
Merged
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Sep 3, 2021
@kibanamachine
[RAC] Add loading and empty states to the alerts table - Take II (ela…
343b22c 
…stic#110504)

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine kibanamachine mentioned this pull request Sep 3, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.15
✅ 7.x

The backport PRs will be merged automatically after passing CI.

@afgomez afgomez deleted the 104923-table-empty-state-reprise branch September 3, 2021 12:14
kibanamachine added a commit that referenced this pull request Sep 3, 2021
@kibanamachine
[RAC] Add loading and empty states to the alerts table - Take II (#11…
b3227b7 
…0504) (#111109)

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Alejandro Fernández Gómez <[email protected]>
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 7, 2021
kibanamachine added a commit that referenced this pull request Sep 7, 2021
@kibanamachine
[RAC] Add loading and empty states to the alerts table - Take II (#11…
d678761 
…0504) (#111110)

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Alejandro Fernández Gómez <[email protected]>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@machadoum machadoum machadoum approved these changes

@weltenwort weltenwort weltenwort approved these changes

@mdefazio mdefazio Awaiting requested review from mdefazio

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Observability RAC Feature:RAC label obsolete release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.15.0 v7.16.0 v8.0.0
Projects
None yet
Milestone
No milestone
6 participants
@afgomez @elasticmachine @machadoum @mdefazio @kibanamachine @weltenwort