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

[App Search] Crawler: do not render page header while loading domains #108078

Merged
merged 2 commits into from
Aug 10, 2021

Conversation

cee-chen
Copy link
Contributor

Summary

@byronhulcher (cc @daveyholler) UX suggestion I noticed while QAing crawler engines locally. The "Loading..." title threw me off a bit, and it felt a little heavier/more distracting than just a simple logo loading icon.

This is a totally optional suggestion, so feel free to let me know if you don't prefer it and I can just close this PR; I just thought this would be quicker/easier to open in a PR than to try and describe in Slack.

Before

before

After

after

Checklist

@cee-chen cee-chen added release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v7.15.0 labels Aug 10, 2021
@cee-chen cee-chen requested review from byronhulcher and a team August 10, 2021 17:14
pageTitle: displayDomainUrl,
rightSideItems: [<ManageCrawlsPopover />, <CrawlerStatusIndicator />],
}}
pageChrome={getEngineBreadcrumbs([CRAWLER_TITLE, domain?.url || '...'])}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a pattern used by Workplace Search as well, FWIW

Comment on lines +43 to +44
dataLoading
? undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also a pattern used in our codebase - I think more in WS than AS, since AS has more names that can be derived from the URL vs IDs

Copy link
Contributor

@byronhulcher byronhulcher 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 improving this @constancecchen!

@cee-chen cee-chen enabled auto-merge (squash) August 10, 2021 17:24
@cee-chen
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
enterpriseSearch 2.1MB 2.1MB -146.0B

History

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

@cee-chen cee-chen merged commit 40766dc into elastic:master Aug 10, 2021
@cee-chen cee-chen deleted the crawler-domain-loading branch August 10, 2021 22:01
@cee-chen
Copy link
Contributor Author

looking nervously at auto-backport bot

@tylersmalley tylersmalley added auto-backport Deprecated - use backport:version if exact versions are needed and removed auto-backport Deprecated - use backport:version if exact versions are needed labels Aug 10, 2021
@cee-chen
Copy link
Contributor Author

Looks like there's a GH actions outage (thanks Tyler!), I'll just manually backport

@cee-chen cee-chen removed the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 10, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 108078

@cee-chen cee-chen added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 10, 2021
@cee-chen
Copy link
Contributor Author

Trying again since #107953 merged

@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 10, 2021
kibanamachine added a commit that referenced this pull request Aug 11, 2021
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 11, 2021
…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (101 commits)
  [ML] APM Latency Correlations: Field/value candidates prioritization (elastic#107370)
  [Reporting] Add lenience to a test on the order of asserted logs (elastic#108135)
  [Lens] fix do not submit invalid query in filtered metric (elastic#107542)
  skip flaky test (elastic#108043)
  fix newly introduced type error (elastic#107593)
  [Reporting] server side code clean up (elastic#106940)
  [build_ts_refs] improve caches, allow building a subset of projects (elastic#107981)
  [APM] Add new ftr_e2e to kibana CI and remove current e2e tests. (elastic#107593)
  add manage rules link to alerts dropdown (elastic#107950)
  [ML] Enable Index data visualizer document count chart to update time range query (elastic#106438)
  [Security Solutions][Detection Engine] Fixes "undefined" crash for author field by adding a migration for it (elastic#107230)
  [Actions UI] Fixed Jira Api token label. (elastic#107776)
  [Alerting UI] Fixed display permissions for edit/delete buttons when user has read only access. (elastic#107996)
  [Maps] fix code owners (elastic#108106)
  Update EMS landing page url (elastic#108102)
  Do not render page header for loading domains (elastic#108078)
  Update dependency @elastic/charts to v33.2.2 (elastic#107939)
  [APM] Display throughput as tps (instead of tpm) when bucket size < 60 seconds (elastic#107850)
  [Fleet] Fix all category count (elastic#108089)
  [Security Solution][Bug] - Disable alert table RBAC until fields sorted (elastic#108034)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/export_types/common/generate_png.ts
#	x-pack/plugins/reporting/server/lib/screenshots/index.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants