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

[Workplace Search] Convert Overview & Security pages to new page template #102444

Merged
merged 3 commits into from
Jun 17, 2021

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jun 17, 2021

Summary

Follow up to #102170 - converts more Workplace Search pages to the new KibanaPageTemplate. I'm attempting to break up the WS layout conversion into smaller, easier to review chunks.

This PR handles the simplest 2 pages without any sub-navigation. As always, follow along by commit!

Screencap

screencap

NOTE: The current UI shows little dropdown arrows next to the current links w/ sub-navs - this will go away once the actual sub-navs get implemented

Checklist

- Fix extra spacing caused by hidden onboarding steps

- Default page title to "Organization overview" instead of to onboarding title which flashes during loading

- Prefer shallow over mount (will matter later, when WorkplaceSearchPageTemplate nav includes more kea logic)
+ misc ux enhancement disabling header actions while data is still loading
@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 17, 2021
@cee-chen cee-chen requested a review from a team June 17, 2021 04:04
Comment on lines -99 to -100
{errorConnecting ? (
<ErrorState />
Copy link
Member Author

@cee-chen cee-chen Jun 17, 2021

Choose a reason for hiding this comment

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

This error state isn't going away long term - I'll handle 404 & 500 states in the final commit/pass that cleans up all the layouts and so forth. Basically it's going to get handled in 1 place at the top-level going forward like how App Search manages it.

Comment on lines -24 to -25
describe('non-happy-path states', () => {
it('isLoading', () => {
Copy link
Member Author

@cee-chen cee-chen Jun 17, 2021

Choose a reason for hiding this comment

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

Mostly just indenting changes in this file due to deleting the happy/non-happy path blocks - I recommend viewing the diff with whitespace changes off.

Also as a heads up, I'm removing most isLoading tests because they're now a simple/basic prop pass-through to the underlying page template, which already has its own unit tests for loading state.

Comment on lines +83 to +84
const headerActions = getPageHeaderActions(wrapper);
headerActions.find('[data-test-subj="SaveSettingsButton"]').prop('onClick')!({} as any);
Copy link
Member Author

Choose a reason for hiding this comment

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

new test_helpers already paying off in spades

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

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

Copy link
Contributor

@yakhinvadim yakhinvadim left a comment

Choose a reason for hiding this comment

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

This looks great! Love the simplification and DRY-ing out of many common concerns.

I didn't QA this PR but will do a QA once all related PRs are merged.

pageHeader={{
pageTitle: PRIVATE_SOURCES,
description: PRIVATE_SOURCES_DESCRIPTION,
rightSideItems: headerActions,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit (not for this PR): What do you think about renaming rightSideItems to just sideItems or actions?
"Right" might not be a correct name in some environments: possibly with rtl languages, on mobile, or screen readers.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an EuiPageTemplate prop, we don't control it! https://elastic.github.io/eui/#/layout/page and https://elastic.github.io/eui/#/layout/page-header for reference

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry still waking up so I just realized I didn't fully answer you. I totally agree with you on the rtl thing - I like sideItems personally, actions feels a little prescriptive since you can have just plain text or notifications and not buttons there. But yeah I think this is totally worth suggesting to the EUI folks, if you wanna ping them! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, didn't know that! Well, ok, then. 🤷

@cee-chen cee-chen merged commit 1627240 into elastic:master Jun 17, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 17, 2021
…late (elastic#102444)

* Convert WS Overview page to new page template

* Misc Overview refactors

- Fix extra spacing caused by hidden onboarding steps

- Default page title to "Organization overview" instead of to onboarding title which flashes during loading

- Prefer shallow over mount (will matter later, when WorkplaceSearchPageTemplate nav includes more kea logic)

* Convert Security page to new page template

+ misc ux enhancement disabling header actions while data is still loading
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

@cee-chen cee-chen deleted the kibana-page-template-ws-1 branch June 17, 2021 16:32
kibanamachine added a commit that referenced this pull request Jun 17, 2021
…late (#102444) (#102530)

* Convert WS Overview page to new page template

* Misc Overview refactors

- Fix extra spacing caused by hidden onboarding steps

- Default page title to "Organization overview" instead of to onboarding title which flashes during loading

- Prefer shallow over mount (will matter later, when WorkplaceSearchPageTemplate nav includes more kea logic)

* Convert Security page to new page template

+ misc ux enhancement disabling header actions while data is still loading

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
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.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants