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.

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

[Enterprise Search] Share Loading component #83246

Merged
merged 5 commits into from
Nov 13, 2020
Merged

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Nov 12, 2020

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 JasonStoltz and a team November 12, 2020 05:19
@cee-chen
Copy link
Member Author

@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.

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!

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

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

@cee-chen
Copy link
Member Author

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
Member Author

@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
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

…h/components/engine/engine_router.tsx

Co-authored-by: Scotty Bollinger <[email protected]>
@kibanamachine
Copy link
Contributor

💚 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 pushed a commit that referenced this pull request Nov 13, 2020
* 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 <[email protected]>

Co-authored-by: Scotty Bollinger <[email protected]>

Co-authored-by: Scotty Bollinger <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 16, 2020
* 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
Labels
Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants