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.

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

[Workplace Search] Convert Sources pages to new page template (+ personal dashboard) #102592

Merged
merged 15 commits into from
Jun 22, 2021
Merged

[Workplace Search] Convert Sources pages to new page template (+ personal dashboard) #102592

merged 15 commits into from
Jun 22, 2021

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jun 18, 2021 •
edited by yakhinvadim
Loading

Summary

Follow up to #102170 - this is the last PR for converting a chunk of Workplace Search views to the new KibanaPageTemplate. It's the largest and most complex one since Sources is already the most complex set of views, and requires accounting for the Personal Dashboard layout + updating that as well. As always, follow along by commit and disable whitespace diffs, etc.

Screencaps

Organization Sources

organization_sources

Personal Dashboard

private_sources

(Not sure why this one spent so much time loading or had an error on the content sources page...maybe because I'd just added the slack connector?)

QA

⚠️ I need QA help on this one! I'm fairly sure I QA'd everything, but it's strongly possible I missed something from just not knowing enough about Workplace Search. Please pull this down locally and check to make sure:

  • all pages look visually fine and load as expected
  • empty states look fine as well as filled states
  • document titles+breadcrumbs update as expected
  • reloading/refreshing the app on all pages works and doesn't cause strange behavior or crashes due to (e.g.) incorrect logic mount order

Checklist

@cee-chen
Refactor PersonalDashboardLayout to more closely match new page template
750f227 
- Remove references to enterpriseSearchLayout CSS (which will be removed in an upcoming PR)

- Prefer to lean more heavily on default EuiPage props/CSS/etc.

- Handle conditional sidebar logic in this layout rather than passing it in as a prop

- Update props & DRY concerns to more closely match WorkplaceSearchPageTemplate
  - e.g. isLoading & pageChrome (mostly for document titles)
  - make FlashMessage and readOnlyMode work OOTB w/o props)
@cee-chen
Convert Source subnav to EuiSideNav format
b4ebfa4 
+ update PrivateSourcesSidebar to use EuiSIdeNav
@cee-chen
Update routers
1fffcf6 
- removing wrapping layouts, flash messages, chrome/telemetry
@cee-chen
Refactor SourceRouter into shared layout component
637ba53 
- Remove license callout, page header, and page chrome/telemetry

- NOTE: The early page isLoading behavior (lines 51-) is required to prevent a flash of a completely empty page (instead of preserving the layout/side nav while loading). We cannot let the page fall through to the route because some routes are conditionally rendered based on isCustomSource.

- FWIW: App Search has a similar isLoading early return with its Engine sub nav, and also a similar AnalyticsLayout for DRYing out repeated concerns/UI elements within Analytics subroutes.
@cee-chen
Convert all single source views to new page template
7bb4891 
- Mostly removing isLoading tests
- NOTE: Schema page could *possibly* use the new isEmptyState/emptyState page template props, but would need some layout reshuffling
@cee-chen
Convert Add Source pages to conditional page templates
91c51f9 
- Opted to give these pages their own conditional layout logic - this could possibly be DRY'd out

- There is possibly extra cleanup here on this file that could have been done (e.g. empty state, titles, etc.) in light of the new templates - but I didn't want to spend extra time here and went with creating as few diffs as possible
@cee-chen
Convert separate Organization Sources & Private Sources views to new …
7f42518 
…page templates

+ fix Link to EuiButtonTo on Organization Sources view
@cee-chen
Update Account Settings with personal layout + write tests
6c305e5 
+ add related KibanaLogic branch coverage
@cee-chen cee-chen added release_note:skip Skip the PR/issue when compiling release notes v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Jun 18, 2021
@cee-chen cee-chen requested review from a team June 18, 2021 02:23
cee-chen
cee-chen commented Jun 18, 2021
View reviewed changes
...terprise_search/public/applications/workplace_search/views/content_sources/source_router.tsx Show resolved Hide resolved
...arch/public/applications/workplace_search/views/content_sources/components/source_layout.tsx Show resolved Hide resolved
...arch/public/applications/workplace_search/views/content_sources/components/schema/schema.tsx Show resolved Hide resolved
...pplications/workplace_search/views/content_sources/components/add_source/add_source_list.tsx Show resolved Hide resolved
...pplications/workplace_search/views/content_sources/components/add_source/add_source_list.tsx Show resolved Hide resolved
...search/public/applications/workplace_search/views/account_settings/account_settings.test.tsx Show resolved Hide resolved
x-pack/plugins/enterprise_search/public/applications/shared/kibana/kibana_logic.test.ts Show resolved Hide resolved
@yakhinvadim

This comment has been minimized.

Sign in to view
@yakhinvadim
Copy link
Contributor

@constancecchen I finished the QA and everything except already posted comments looks great! Thank you so much for handling this update! Looking forward to the last PR! 🎉

@cee-chen
[Proposal] Update schema errors routing to better work with nav/bread…
b6a62b6 
…crumbs

- `exact` is required to make the parent schemas/ not gobble schema/{errorId}

- added bonus breadcrumb for nicer schema navigation UX

- No tests need to update AFAICT
@cee-chen
Copy link
Member Author

Thanks so much for the super thorough QA Vadim!!! Just to confirm, no issues on the personal dashboard either right?

If not, I guess we get to wait for EUI! 🎉

yakhinvadim
yakhinvadim approved these changes Jun 18, 2021
View reviewed changes
Copy link
Contributor

@yakhinvadim yakhinvadim left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just to confirm, no issues on the personal dashboard either right?

Yep, no issues!

Approving since all comments are resolved. 💃

This was referenced Jun 19, 2021
Merged
Merged
@cee-chen cee-chen enabled auto-merge (squash) June 21, 2021 21:17
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1454 1455 +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 2.1MB 2.1MB -682.0B

History

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

@cee-chen cee-chen merged commit 138bd0d into elastic:master Jun 22, 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 102592

@cee-chen cee-chen deleted the kibana-page-template-ws-4 branch June 22, 2021 01:21
cee-chen pushed a commit to cee-chen/kibana that referenced this pull request Jun 22, 2021
@cee-chen
[Workplace Search] Convert Sources pages to new page template (+ pers…
5d3b193 
…onal dashboard) (elastic#102592)

* Refactor PersonalDashboardLayout to more closely match new page template

- Remove references to enterpriseSearchLayout CSS (which will be removed in an upcoming PR)

- Prefer to lean more heavily on default EuiPage props/CSS/etc.

- Handle conditional sidebar logic in this layout rather than passing it in as a prop

- Update props & DRY concerns to more closely match WorkplaceSearchPageTemplate
  - e.g. isLoading & pageChrome (mostly for document titles)
  - make FlashMessage and readOnlyMode work OOTB w/o props)

* Convert Source subnav to EuiSideNav format

+ update PrivateSourcesSidebar to use EuiSIdeNav

* Update routers

- removing wrapping layouts, flash messages, chrome/telemetry

* Refactor SourceRouter into shared layout component

- Remove license callout, page header, and page chrome/telemetry

- NOTE: The early page isLoading behavior (lines 51-) is required to prevent a flash of a completely empty page (instead of preserving the layout/side nav while loading). We cannot let the page fall through to the route because some routes are conditionally rendered based on isCustomSource.

- FWIW: App Search has a similar isLoading early return with its Engine sub nav, and also a similar AnalyticsLayout for DRYing out repeated concerns/UI elements within Analytics subroutes.

* Convert all single source views to new page template

- Mostly removing isLoading tests
- NOTE: Schema page could *possibly* use the new isEmptyState/emptyState page template props, but would need some layout reshuffling

* Convert Add Source pages to conditional page templates

- Opted to give these pages their own conditional layout logic - this could possibly be DRY'd out

- There is possibly extra cleanup here on this file that could have been done (e.g. empty state, titles, etc.) in light of the new templates - but I didn't want to spend extra time here and went with creating as few diffs as possible

* Convert separate Organization Sources & Private Sources views to new page templates

+ fix Link to EuiButtonTo on Organization Sources view

* Update Account Settings with personal layout + write tests

+ add related KibanaLogic branch coverage

* [UX feedback] Do not render page headers while loading on Overview & Sources pages

* [PR feedback] Breadcrumb errors/fallbacks

* [Proposal] Update schema errors routing to better work with nav/breadcrumbs

- `exact` is required to make the parent schemas/ not gobble schema/{errorId}

- added bonus breadcrumb for nicer schema navigation UX

- No tests need to update AFAICT

* Ignore Typescript error on soon-to-come EUI prop
# Conflicts:
#	x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/source_settings.tsx
@cee-chen cee-chen mentioned this pull request Jun 22, 2021
Merged
cee-chen pushed a commit that referenced this pull request Jun 22, 2021
[Workplace Search] Convert Sources pages to new page template (+ pers…
e265826 
…onal dashboard) (#102592) (#102843)

* Refactor PersonalDashboardLayout to more closely match new page template

- Remove references to enterpriseSearchLayout CSS (which will be removed in an upcoming PR)

- Prefer to lean more heavily on default EuiPage props/CSS/etc.

- Handle conditional sidebar logic in this layout rather than passing it in as a prop

- Update props & DRY concerns to more closely match WorkplaceSearchPageTemplate
  - e.g. isLoading & pageChrome (mostly for document titles)
  - make FlashMessage and readOnlyMode work OOTB w/o props)

* Convert Source subnav to EuiSideNav format

+ update PrivateSourcesSidebar to use EuiSIdeNav

* Update routers

- removing wrapping layouts, flash messages, chrome/telemetry

* Refactor SourceRouter into shared layout component

- Remove license callout, page header, and page chrome/telemetry

- NOTE: The early page isLoading behavior (lines 51-) is required to prevent a flash of a completely empty page (instead of preserving the layout/side nav while loading). We cannot let the page fall through to the route because some routes are conditionally rendered based on isCustomSource.

- FWIW: App Search has a similar isLoading early return with its Engine sub nav, and also a similar AnalyticsLayout for DRYing out repeated concerns/UI elements within Analytics subroutes.

* Convert all single source views to new page template

- Mostly removing isLoading tests
- NOTE: Schema page could *possibly* use the new isEmptyState/emptyState page template props, but would need some layout reshuffling

* Convert Add Source pages to conditional page templates

- Opted to give these pages their own conditional layout logic - this could possibly be DRY'd out

- There is possibly extra cleanup here on this file that could have been done (e.g. empty state, titles, etc.) in light of the new templates - but I didn't want to spend extra time here and went with creating as few diffs as possible

* Convert separate Organization Sources & Private Sources views to new page templates

+ fix Link to EuiButtonTo on Organization Sources view

* Update Account Settings with personal layout + write tests

+ add related KibanaLogic branch coverage

* [UX feedback] Do not render page headers while loading on Overview & Sources pages

* [PR feedback] Breadcrumb errors/fallbacks

* [Proposal] Update schema errors routing to better work with nav/breadcrumbs

- `exact` is required to make the parent schemas/ not gobble schema/{errorId}

- added bonus breadcrumb for nicer schema navigation UX

- No tests need to update AFAICT

* Ignore Typescript error on soon-to-come EUI prop
# Conflicts:
#	x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/source_settings.tsx
@cee-chen cee-chen mentioned this pull request Jun 23, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottybollinger scottybollinger scottybollinger left review comments

@yakhinvadim yakhinvadim yakhinvadim approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cee-chen @yakhinvadim @kibanamachine @scottybollinger