[Enterprise Search] Move header actions menu to be rendered after the main app + share its store #78691
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- This is anticipation of upcoming renderHeaderActions changes which will require these helpers (particularly the unmount portion, for some reason renderHeaderAction tests fail if apps are not unmounted properly) - Add mount/unmount helper (but leave first test the same so that devs can see the normal/expected usage) - Add missing EnterpriseSearch app test - Add mockContainer var for brevity, pull out MockApp in anticipation of future usage
- a light wrapper/helper around params.setHeaderActionMenu
+ update renderHeaderActions - move to main renderApp block to reflect that its relationship with renderApp ('child'/should be called within renderApp/shares dependencies).
- We need to update WS tests to setMockValues so that it doesn't override the renderHeaderActions mock + bonus - add setMockActions as well because since we're already here + bonus - update App Search index tests as well to match + ?? - for some reason tests were failing beceause react router wasn't mocked properly - requireActual seems to fix that
- set in elastic@9d993d8 - WorkplaceSearchHeaderActions should now still have the correct URL - and also now be able to access all Kea logic set up by the main app :)
cee-chen
added
Feature:Plugins
release_note:skip
|
Haha fffff. I forgot updating the App Search test would trigger CODEOWNERS. Oh well, worth CCing @JasonStoltz in on this one in any case since we discussed the race condition together in-depth! |
|
For a working example of header apps being able to access Kea stores, you can check out my header-actions-store-example branch. 🎉 |
scottybollinger
approved these changes
Sep 28, 2020
| import { ApplicationStart, ChromeBreadcrumb } from 'src/core/public'; | ||
|
|
||
| export interface IKibanaValues { | ||
| config: { host?: string }; | ||
| navigateToUrl: ApplicationStart['navigateToUrl']; | ||
| setBreadcrumbs(crumbs: ChromeBreadcrumb[]): void; | ||
| setDocTitle(title: string): void; | ||
| renderHeaderActions(HeaderActions: FC): void; |
There was a problem hiding this comment.
TIL typing this way 💯
There was a problem hiding this comment.
It felt weird importing all of React just for React.FC so I destructured it out. Totally agreed it looks weirdly naked though haha
byronhulcher
approved these changes
Sep 29, 2020
|
Thanks Byron!! 🙇♀️ |
💚 Build SucceededMetrics [docs]async chunks size
distributable file count
page load bundle size
History
To update your PR or re-run it, just comment with: |
cee-chen
pushed a commit
that referenced
this pull request
Sep 29, 2020
… main app + share its store (#78691) (#78866) * [Setup] Refactor renderApp tests - This is anticipation of upcoming renderHeaderActions changes which will require these helpers (particularly the unmount portion, for some reason renderHeaderAction tests fail if apps are not unmounted properly) - Add mount/unmount helper (but leave first test the same so that devs can see the normal/expected usage) - Add missing EnterpriseSearch app test - Add mockContainer var for brevity, pull out MockApp in anticipation of future usage * Store renderHeaderActions in KibanaLogic - a light wrapper/helper around params.setHeaderActionMenu + update renderHeaderActions - move to main renderApp block to reflect that its relationship with renderApp ('child'/should be called within renderApp/shares dependencies). * Update WorkplaceSearch to render its header actions from app, not plugin.ts * Update WorkplaceSearch tests (+ bonus refactor) - We need to update WS tests to setMockValues so that it doesn't override the renderHeaderActions mock + bonus - add setMockActions as well because since we're already here + bonus - update App Search index tests as well to match + ?? - for some reason tests were failing beceause react router wasn't mocked properly - requireActual seems to fix that * 🔥 Remove temporary externalUrl workaround - set in 9d993d8 - WorkplaceSearchHeaderActions should now still have the correct URL - and also now be able to access all Kea logic set up by the main app :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This is (hopefully) the final PR related to our Kea/store refactors, and solves the app race condition discussed in #78368 (comment). 🎉
What changed:
plugin.ts. They should instead do so from their respective app indexes: 6925b13As always, I recommend following along by commit.
QA/Regression testing
http://localhost:3002/ws/searchChecklist