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] Add Workplace Search side navigation #74894

Merged
merged 17 commits into from
Aug 14, 2020

Conversation

scottybollinger
Copy link
Contributor

@scottybollinger scottybollinger commented Aug 12, 2020

Summary

closes https://github.com/elastic/workplace-search-team/issues/1190

EDIT: After discussions with product, it was decided that the navigation should be hidden and the code merge incrementally but not exposed until a later date.
image

This implements a navigation component that Workplace Search can use for their sidebar navigation:

image

Currently all links except the existing 'overview' pages simply link out externally/in a new tab to Workplace Search.

Checklist

@scottybollinger
Copy link
Contributor Author

scottybollinger commented Aug 12, 2020

@jonasll @brianearwood does it make sense to remove the "Launch Workplace Search" button with this PR since all top-level functionality is linkable from the sidebar?
Workplace Search - Elastic 2020-08-12 14-38-11

@scottybollinger
Copy link
Contributor Author

@elasticmachine merge upstream

@brianearwood
Copy link

Thanks, Scotty! The navigation looks good 👍

does it make sense to remove the "Launch Workplace Search" button with this PR since all top-level functionality is linkable from the sidebar?

It makes sense to me. Now that all of the navigation is in place, the "Launch Workplace Search" button makes me feel like I'm not already in Workplace Search, which is confusing. I'd remove it.

@scottybollinger
Copy link
Contributor Author

@elasticmachine merge upstream

@jonasll
Copy link

jonasll commented Aug 12, 2020

Is this a 7.10 merge target? If so, my answer will differ on whether or not it makes sense to drop it.

@scottybollinger
Copy link
Contributor Author

Is this a 7.10 merge target? If so, my answer will differ on whether or not it makes sense to drop it.

Yes, the plan is to merge this into 7.10

@jonasll
Copy link

jonasll commented Aug 13, 2020

If that's the case, I worry a little bit about the jumpy nature of the experience if we don't make every link in the sidebar followed by an "external link icon". If I click on one of those navigation icons, I definitely do not expect to be taken to an entirely new dashboard with its own navigation. Is App Search following a similar path? @johnbarrierwilson

Copy link
Member

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

Code LGTM Scotty! I'll wait to approve for now until we figure out any product/design changes or requirements from Jonas etc. FWIW, I hadn't heard of any requirement from @johnbarrierwilson or @daveyholler on adding an icon to indicate an external vs non-external URL, but definitely appreciate that UX point and will defer to y'all on preference.

Also as a quick heads up Scotty - if Workplace Search has any nested/indented navigation coming up, I do have a dev branch that adds the styling and logic for that behavior - let me know if you need it sooner rather than later!

@@ -6873,7 +6873,6 @@
"xpack.enterpriseSearch.workplaceSearch.overviewOnboardingUsersCard.description": "検索できるように、同僚をこの組織に招待します。",
"xpack.enterpriseSearch.workplaceSearch.overviewOnboardingUsersCard.title": "ユーザーと招待",
"xpack.enterpriseSearch.workplaceSearch.overviewUsersCard.title": "検索できるように、同僚を招待しました。",
"xpack.enterpriseSearch.workplaceSearch.productCta": "Workplace Searchの起動",
"xpack.enterpriseSearch.workplaceSearch.productName": "Workplace Search",
Copy link
Member

Choose a reason for hiding this comment

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

Oh wow, I hadn't realized we already received translations for our plugin!!!! 🎉 🎉 🎉

@daveyholler
Copy link
Contributor

I don't disagree about the jumpy nature of links opening externally, but I'm also a little hesitant to add the external link icon to every link. I think, visually, it'll get pretty busy. Especially in table views with a stack of links that open in a new window. Opinion is strongly voiced and lightly held.

@cee-chen
Copy link
Member

Maybe we can circle back to how many views/links we've ported over about 2 weeks before 7.10 feature freeze and see how many outgoing/external links we have left at that point? And use those numbers to determine then if we want to add 'new window' icons?

For now, the route for groups was added to avoid having comment out the code. Will add the groups component in a future PR
Since there is no nav, the padding was missing and the view looked off. Reverting to the 7.9, centered column view
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
enterpriseSearch 196 +5 191

async chunks size

id value diff baseline
enterpriseSearch 359.6KB +51.9KB 307.7KB

History

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

@scottybollinger scottybollinger merged commit ac04e05 into elastic:master Aug 14, 2020
@scottybollinger scottybollinger deleted the ws-nav branch August 14, 2020 20:28
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 17, 2020
* master: (24 commits)
  [ML] Functional tests - skip regression and classification tests
  [Ingest Manager] fix removing ingest pipelines from elasticsearch (elastic#75092)
  move tests for placeholder indices to setup (elastic#75096)
  [jest] temporarily extend default test timeout (elastic#75118)
  [cli] remove reference to removed --optimize flag (elastic#75083)
  skip flaky suite (elastic#75044)
  Adding /etc/rc.d/init.d/functions to the init script when present to … (elastic#22985)
  [jenkins] add pipeline for hourly security solution cypress tests (elastic#75087)
  [Reporting/Flaky Test] Skip test for paging list of reports (elastic#75075)
  remove .kbn-optimizer-cache upload (elastic#75086)
  skip flaky suite (elastic#74814)
  Actions add proxy support (elastic#74289)
  [ILM] TS conversion of Edit policy components (elastic#74747)
  [Resolver] simulator tests select elements directly instead of using descendant selectors. (elastic#75058)
  [Enterprise Search] Add Workplace Search side navigation (elastic#74894)
  [Security solution] Sourcerer: Kibana index pattern selector for security views (elastic#74706)
  [Logs UI] Remove apollo deps from log link-to routes (elastic#74502)
  [Maps] add map configurations to docker list (elastic#75035)
  [functional test][saved objects] update tests for additional copy saved objects to space (elastic#74907)
  Make the alerts plugin support generics (elastic#72716)
  ...
scottybollinger added a commit to scottybollinger/kibana that referenced this pull request Aug 17, 2020
* Add routes

* Add version for use in doc link

* Set up basic router layout + WorkplaceSearchNav

* Update views to account for Layout

* Move version to common folder

* Fix version path

* Remove product button

No longer needed since we have all top-level app links in Kibana as a part of this PR

* You know, for search

* Remove comment

* Remove unused i18n properties from JSON

Tests were failing after removing component:
https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/67797/execution/node/382/log/

* Revert button and i18n copy  removal

This reverts commit ba05351.

* Move Overview out of layout to hide nav

For now, the route for groups was added to avoid having comment out the code. Will add the groups component in a future PR

* Revert layout changes to Overview

Since there is no nav, the padding was missing and the view looked off. Reverting to the 7.9, centered column view

* Remove extra Overview component

Was causing tests to fail

* Revert error state to use EuiPage

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
cee-chen pushed a commit that referenced this pull request Aug 17, 2020
…5191)

* Add routes

* Add version for use in doc link

* Set up basic router layout + WorkplaceSearchNav

* Update views to account for Layout

* Move version to common folder

* Fix version path

* Remove product button

No longer needed since we have all top-level app links in Kibana as a part of this PR

* You know, for search

* Remove comment

* Remove unused i18n properties from JSON

Tests were failing after removing component:
https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/67797/execution/node/382/log/

* Revert button and i18n copy  removal

This reverts commit ba05351.

* Move Overview out of layout to hide nav

For now, the route for groups was added to avoid having comment out the code. Will add the groups component in a future PR

* Revert layout changes to Overview

Since there is no nav, the padding was missing and the view looked off. Reverting to the 7.9, centered column view

* Remove extra Overview component

Was causing tests to fail

* Revert error state to use EuiPage

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants