Skip to content

[Enterprise Search] Share Loading component #83246

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
Merged
merged 5 commits into from
Nov 13, 2020
Merged

[Enterprise Search] Share Loading component #83246

merged 5 commits into from
Nov 13, 2020

Conversation

@cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Nov 12, 2020 •
edited
Loading

Summary

[Edit: PR was updated to have both App Search and Workplace Search share the same basic Loading component]

I'm adding a super basic loading spinner for views where skeletal views don't make sense. Also to make our migration a little bit faster so we don't have to think too much / change too much WRT dataLoading & early returns.

desktop

mobile

QA

  • To QA, you can remove the if dataLoading conditional on engine_router.tsx so it always returns Loading and go to any engine page.

Checklist

@cee-chen cee-chen added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Nov 12, 2020
@cee-chen cee-chen requested review from a team and JasonStoltz November 12, 2020 05:19
@cee-chen
Copy link
Contributor Author

cee-chen commented Nov 12, 2020

@daveyholler FYI on this. It's not as fancy as App Search's current loader but if we really want that I'd like to ask we postpone until 7.13+ and later investigate how to port a nicer animation without a 3rd party animation lib dependency.

JasonStoltz
JasonStoltz approved these changes Nov 12, 2020
View reviewed changes
Copy link
Member

@JasonStoltz JasonStoltz left a comment

Choose a reason for hiding this comment

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

Nice!

daveyholler
daveyholler approved these changes Nov 12, 2020
View reviewed changes
Copy link
Contributor

@daveyholler daveyholler left a comment

Choose a reason for hiding this comment

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

This is perfectly acceptable. Thanks!!!

@cee-chen cee-chen force-pushed the loading branch 2 times, most recently from 1f20e65 to 2e5ad24 Compare November 12, 2020 16:03
@scottybollinger
Copy link
Contributor

scottybollinger commented Nov 12, 2020

I wonder if it's worth just sharing the one we have for DRY sake?

@cee-chen
Copy link
Contributor Author

cee-chen commented Nov 12, 2020

WHAT! 🤯 I totally missed that lol, thanks Scotty.

The only reason why I can think we wouldn't is if we do concretely plan on backporting the app search specific loader which has our logo and fancy animations. Lemme check w/ Davey rq.

@cee-chen
Copy link
Contributor Author

cee-chen commented Nov 12, 2020

@scottybollinger Here's WS with the new Loading component:

Screen Shot 2020-11-12 at 11 11 16 AM

FYI that I use position absolute to center which works well with all our Layout views but less so with the non-Layout WS overview, so I added a temporary wrapper: a25d7dd

Screen Shot 2020-11-12 at 11 16 00 AM

Let me know if that looks good to you!

@cee-chen cee-chen changed the title [App Search] Add basic Loading component [Enterprise Search] Share Loading component Nov 12, 2020
scottybollinger
scottybollinger approved these changes Nov 12, 2020
View reviewed changes
Copy link
Contributor

@scottybollinger scottybollinger left a comment

Choose a reason for hiding this comment

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

This is great. Works perfectly locally. One suggested nit

@scottybollinger
Update x-pack/plugins/enterprise_search/public/applications/app_searc…
dc6e900 
…h/components/engine/engine_router.tsx

Co-authored-by: Scotty Bollinger <scotty.bollinger@elastic.co>
@kibanamachine
Copy link
Contributor

kibanamachine commented Nov 13, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1040 1039 -1

Async chunks

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

id before after diff
enterpriseSearch 696.3KB 697.3KB +1.1KB
infra 2.5MB 2.5MB -8.9KB
total -7.8KB

History

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

@cee-chen cee-chen merged commit 35faf8c into elastic:master Nov 13, 2020
@cee-chen cee-chen deleted the loading branch November 13, 2020 01:02
@cee-chen cee-chen mentioned this pull request Nov 13, 2020
Merged
cee-chen pushed a commit that referenced this pull request Nov 13, 2020
@scottybollinger
[Enterprise Search] Share Loading component (#83246) (#83366)
20aa1c4 
* Add basic Loading component

- for pages with shifting layouts or where skeletal loading doesn't make sense

* Move Loading to shared components

* Update WS to use new shared Loading component

* Fix for non-Layout WS Overview page

* Update x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/engine_router.tsx

Co-authored-by: Scotty Bollinger <scotty.bollinger@elastic.co>

Co-authored-by: Scotty Bollinger <scotty.bollinger@elastic.co>

Co-authored-by: Scotty Bollinger <scotty.bollinger@elastic.co>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 16, 2020
@gmmorris
Merge branch 'master' into alerts/action-groups-as-conditions
c20db06 
* master:
  [Security Solution][Detections] Adds framework for replacing API schemas (elastic#82462)
  [Enterprise Search] Share Loading component (elastic#83246)
  Consolidates Jest configuration files and scripts (elastic#82671)
  APM header changes (elastic#82870)
  [Security Solutions] Adds a default for indicator match custom query of *:* (elastic#81727)
  [Security Solution] Note 10k object paging limit on Endpoint list (elastic#82889)
  [packerCache] fix gulp usage, don't archive node_modules (elastic#83327)
  Upgrade Node.js to version 12 (elastic#61587)
  [Actions] Removing placeholders and updating validation messages on connector forms (elastic#82734)
  [Fleet] Rename ingest_manager_api_integration tests fleet_api_integration (elastic#83011)
  [DOCS] Updates Discover docs (elastic#82773)
  [ML] Data frame analytics: Adds map view (elastic#81666)
  enables actions scoped within the stack to register at Basic license (elastic#82931)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JasonStoltz JasonStoltz JasonStoltz approved these changes

+2 more reviewers

@daveyholler daveyholler daveyholler approved these changes

@scottybollinger scottybollinger scottybollinger approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.11.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cee-chen @scottybollinger @kibanamachine @daveyholler @JasonStoltz