Skip to content

Conversation

@scottybollinger
Copy link
Contributor

@scottybollinger scottybollinger commented Sep 22, 2020

Summary

This PR adds for the use-case where a user has not configured Enterprise Search but lands on the Product Selector. It also adds a Enterprise Search-level Setup Guide when there is an error connecting.

Product Selector

Setup Guide

(Image to be updated)

QA

  • Ensure there is no enterpriseSearch.host set in the kibana.dev.yml file and visit the Enterprise Search Overview page. Links should say Setup Workplace Search and Setup App Search and link to the setup guide.
  • Ensure there is enterpriseSearch.host set in the kibana.dev.yml file and make sure Enterprise Search is not running. Visit the Enterprise Search Overview page. Error connecting page should render and button should take you to Enterprise Search setup guide.
  • Ensure there is enterpriseSearch.host set in the kibana.dev.yml file and make sure Enterprise Search is running. Visit the Enterprise Search Overview page. Product selector should be shown with links that say Launch Workplace Search and Launch App Search and link to the product overview pages.

Checklist

- Only shows error connectign if host is set
- Removes conditional rendering of cards
- Changes the action text from “Launch” to “Setup”
@scottybollinger scottybollinger added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 22, 2020
src={GettingStarted}
alt={i18n.translate('xpack.enterpriseSearch.enterpriseSearch.setupGuide.videoAlt', {
defaultMessage:
"Getting started with App Search - in this short video we'll guide you through how to get App Search up and running",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in future commit

@scottybollinger scottybollinger marked this pull request as ready for review September 22, 2020 23:10
@scottybollinger scottybollinger requested a review from a team September 22, 2020 23:10
@JasonStoltz JasonStoltz self-assigned this Sep 23, 2020
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.

Left some comments and feedback. Let me know what you think.


import { APP_SEARCH_PLUGIN, WORKPLACE_SEARCH_PLUGIN } from '../../../../../common/constants';

import { SetEnterpriseSearchChrome as SetPageChrome } from '../../../shared/kibana_chrome';
Copy link
Member

Choose a reason for hiding this comment

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

Why do we alias this? I feel like it confuses things a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copying Constance's pattern from other components. If we want to change IMO that is beyond the scope of this PR and we should discuss as a team and change all in a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey! This pattern's on me (also one we use in multiple places, e.g., with telemetry, so def open to discussing and refactoring separately from this PR).

Most of these components Set<Product>Chrome, Set<Product>Telemetry are a very very light wrapper around what's essentially a generic function with a product key passed in. Examples:

I thought it would feel and read more nicely from a dev experience perspective - for example, if you're an App Search dev you know you're already working in the App Search plugin, so no need to be overly specific when calling the component - just rename it to a more generic <SetChrome /> component, or <SendTelemetry />, etc. without having to repeat App Search or pass the app search ID again and again.

Definitely happy to discuss the overall pattern and if you have any thoughts on refactoring, but would very likely be in a separate PR if that sounds cool!

import { shallow } from 'enzyme';

import { SetEnterpriseSearchChrome as SetPageChrome } from '../../../shared/kibana_chrome';
import { SetupGuide as SetupGuideLayout } from '../../../shared/setup_guide';
Copy link
Member

Choose a reason for hiding this comment

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

I feel we should just rename shared/setup_guide to shared/setup_guide_layout.tsx and export SetupGuideLayout. It's confusing having a file named one way but used another way everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copying Constance's pattern from other components. If we want to change IMO that is beyond the scope of this PR and we should discuss as a team and change all in a separate PR

Copy link
Contributor

@cee-chen cee-chen Sep 23, 2020

Choose a reason for hiding this comment

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

This one's on me again, and almost definitely an alias we don't need 🤔 Historical context for this shenanigans:

  1. The (very early) MVP only had a single <SetupGuide /> component
  2. When WS was added, I refactored out the instructions part of the Setup Guide to a reusable component, and left it named as SetupGuide and just alias'ed it
  3. Here we are today and I look like a fool

EDIT: Went back and actually re-looked at what I was doing, I'm +1 for just renaming this SetupGuideLayout or similar. Thanks @JasonStoltz!

Copy link
Contributor

Choose a reason for hiding this comment

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

^ Sorry, to clarify - in a separate PR of course

…e_search/index.test.tsx

Co-authored-by: Jason Stoltzfus <[email protected]>
@cee-chen
Copy link
Contributor

Super duper quick request - can we run the getting_started.png file through https://tinypng.com/? It should shave ~half of the filesize of that image, and maybe prevent Kibana Machine from ⚠️ing at us about our assets size, haha

@scottybollinger
Copy link
Contributor Author

Super duper quick request - can we run the getting_started.png file through https://tinypng.com/? It should shave ~half of the filesize of that image, and maybe prevent Kibana Machine from ⚠️ing at us about our assets size, haha

2 things:

  1. I already ran it through
  2. @daveyholler is going to provide a new image before 7.10. This is a placeholder and I didn't want to hold up the PR while it was being made. Will open a separate PR to add the image once complete

scottybollinger and others added 2 commits September 23, 2020 15:42
Co-authored-by: Constance <[email protected]>
Co-authored-by: Constance <[email protected]>
@cee-chen
Copy link
Contributor

Super good to know re: the images! Also interesting that tinypng re-compressed it for me apparently, normally it's pretty good / only shows single-digit perf improvements when I re-run images through it 🤔 ++ to leaving it alone since Davey will get back to us anyway!

@scottybollinger
Copy link
Contributor Author

Super good to know re: the images! Also interesting that tinypng re-compressed it for me apparently, normally it's pretty good / only shows single-digit perf improvements when I re-run images through it 🤔 ++ to leaving it alone since Davey will get back to us anyway!

Yeah, so apparently I uploaded the wrong one because I still had the TinyPng compressed one in my downloads folder. Updated anyway 😂

@cee-chen
Copy link
Contributor

Haha I've definitely done the same thing before, good to know I'm not the only one!!

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

kibanamachine's doin' a yell, I think these removals should fix them

@cee-chen
Copy link
Contributor

Ooo was gonna ask about the lodash imports as well - you read my mind!! Thanks Scotty, you rock 🚀

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
enterpriseSearch 252 -23 275

async chunks size

id value diff baseline
enterpriseSearch 638.7KB +128.5KB 510.2KB

miscellaneous assets size

id value diff baseline
enterpriseSearch 834.9KB +190.0KB 644.9KB

page load bundle size

id value diff baseline
enterpriseSearch 23.2KB +25.0B 23.2KB

distributable file count

id value diff baseline
default 45942 +1 45941

History

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

@scottybollinger scottybollinger merged commit 6a04a64 into elastic:master Sep 24, 2020
@scottybollinger scottybollinger deleted the scottybollinger/product-selector-fixes branch September 24, 2020 03:55
scottybollinger added a commit to scottybollinger/kibana that referenced this pull request Sep 24, 2020
…tic#78233)

* Add conditional button text

- Only shows error connectign if host is set
- Removes conditional rendering of cards
- Changes the action text from “Launch” to “Setup”

* Add setup guide

* Extract ProductSelector to component

* Update index and add routes

* Change setup guide text

* Fix imports

* Add missing mock

* Update x-pack/plugins/enterprise_search/public/applications/enterprise_search/index.test.tsx

Co-authored-by: Jason Stoltzfus <[email protected]>

* Remove Literals

Co-authored-by: Constance <[email protected]>

* Remove Literals II - The Force Awakens

Co-authored-by: Constance <[email protected]>

* Add back access checks

* Remove hard-coded props 🤦🏼‍♂️

* Remove data-test-subj attr

* Reafactor access check variables

* Remove unused beforeEach

Co-authored-by: Constance <[email protected]>

* Add newline

Co-authored-by: Constance <[email protected]>

* Update image to compressed

* Remove unused things

* Update to new way of using lodash things 🤷🏽‍♀️

Co-authored-by: Jason Stoltzfus <[email protected]>
Co-authored-by: Constance <[email protected]>
scottybollinger added a commit that referenced this pull request Sep 24, 2020
…) (#78381)

* Add conditional button text

- Only shows error connectign if host is set
- Removes conditional rendering of cards
- Changes the action text from “Launch” to “Setup”

* Add setup guide

* Extract ProductSelector to component

* Update index and add routes

* Change setup guide text

* Fix imports

* Add missing mock

* Update x-pack/plugins/enterprise_search/public/applications/enterprise_search/index.test.tsx

Co-authored-by: Jason Stoltzfus <[email protected]>

* Remove Literals

Co-authored-by: Constance <[email protected]>

* Remove Literals II - The Force Awakens

Co-authored-by: Constance <[email protected]>

* Add back access checks

* Remove hard-coded props 🤦🏼‍♂️

* Remove data-test-subj attr

* Reafactor access check variables

* Remove unused beforeEach

Co-authored-by: Constance <[email protected]>

* Add newline

Co-authored-by: Constance <[email protected]>

* Update image to compressed

* Remove unused things

* Update to new way of using lodash things 🤷🏽‍♀️

Co-authored-by: Jason Stoltzfus <[email protected]>
Co-authored-by: Constance <[email protected]>

Co-authored-by: Jason Stoltzfus <[email protected]>
Co-authored-by: Constance <[email protected]>
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.10.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants