-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Conversation
- 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
{errorConnecting ? ( | ||
<ErrorState /> |
There was a problem hiding this comment.
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.
describe('non-happy-path states', () => { | ||
it('isLoading', () => { |
There was a problem hiding this comment.
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.
const headerActions = getPageHeaderActions(wrapper); | ||
headerActions.find('[data-test-subj="SaveSettingsButton"]').prop('onClick')!({} as any); |
There was a problem hiding this comment.
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
There was a problem hiding this 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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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! 👍
There was a problem hiding this comment.
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. 🤷
…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
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…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>
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
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