[Workplace Search] Initial rendering of Org Sources #84164
Conversation
Didn’t have a way to test these when created
No need to do this in 2 places now. There was a race condition where the default logic value for `isOrganization` was set to `false` We don’t need a useEffect call here because the value is synchronous and has no side effects. Calling the method directly fixes the race condition.
It was decided by Product that instead of keying off of `/org` to determine context, that we would now flip it where we key of provate with `/p`. This means that /sources is now organization where before it was personal
This aligns with App Search and allows for easier telemtry and breadcrumbs
As a part of this commit, the approach for rendering subnavs was refactored to align with App Search. There was a bug where some components weren’t rendering properly because the SourceLogic and GroupsLogic files were never unmounting. The reason for this is the subnav components use their respective logic files to get the IDs needed for rendering the subnav links. That is, SourceSubNav would call SourceLogic to get the ID to render the links and would stay rendered for the duration of the user’s time in the app. The result is that users would leave the source details page and navigate to add a new source and the logic file would never reset to a loading state and the UI would break. The fix was to borrow from the pattern App Search uses and pass the subnavs as props. Because App Search only uses a single engines subnav, they only needed one prop. We use multiple props for each subnav. Also, the subnav should not be rendered on the root routes (/sources, /p/sources, and /groups) so conditionals were added to only render the subnavs when not on those root routes.
Missed this in first commit
Before this change, the legacy styles were not ported over. This gives a uniform size for both wrapped and unwrapped icons. The icons are a bit smaller on the add source page but Eui has lowered it’s largest size ‘xxl’ and we would need to write manual overrides. IMO the change is not significant enough to override.
The eui.css file in ent-search is no longer up to date with current EUI and this was broken. The best fix was to use the component that renders as expected
More in a future PR but this makes the majority of things look correct.
scottybollinger
added
v8.0.0
release_note:skip
We have multiple `Layouts` now with the new subnavs
Like the first commit, missed these when porting over routes with no UI.
The columns were way off in Kibana
No longer needed since ‘/org’ is gone
|
@constancecchen when you have some time (next week is totally fine) can you take a look at my approach here? No need to review the entire PR; just wanted to get your thoughts on how I implemented this particular thing. |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
Unknown metric groupsmiscellaneous assets size
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
This is amazing, Scotty! 💯
Everything looks good to me except for increased bundle size:
| id | before | after | diff |
|---|---|---|---|
enterpriseSearch |
746.6KB | 985.0KB |
I couldn't find the obvious source of the increase. The only new import I noticed is telemetry and kibana chrome, but I expect them to already be included in enterpriseSearch bundle.
Do you have ideas?
Please feel free to merge if the bundle size increase is expected and unavoidable.
Thanks Vadim. This is the bulk of the Workplace Search application and my assumption is that the bundle size increased because all of the code in the 6 PRs listed above had not actually been imported into the app until this PR, as they were simply migrated and never injected into the bundle. This PR makes use of all of those components and that increase does not surprise me, although I do think we should, as Constance has suggested, explore code splitting the entire Enterprise Search Kibana plugin at some point. If we have time before 8.0, I can start looking into this. Going to go ahead and merge now. |
* master: (41 commits) [Maps] fix code-owners (elastic#84265) [@kbn/utils] Clean target before build (elastic#84253) [code coverage] collect for oss integration tests (elastic#83907) [APM] Use `asTransactionRate` consistently everywhere (elastic#84213) Attempt to fix incremental build error (elastic#84152) Unskip "Copy dashboards to space" (elastic#84115) Remove expressions.legacy from README (elastic#79681) Expression: Add render mode and use it for canvas interactivity (elastic#83559) [deb/rpm] Move systemd service to /usr/lib/systemd/system (elastic#83571) [Security Solution][Resolver] Allow a configurable entity_id field (elastic#81679) [ML] Space permision checks for job deletion (elastic#83871) [build] Provide ARM build of RE2 (elastic#84163) TSVB should use "histogram:maxBars" and "histogram:barTarget" settings for auto instead of a default 100 buckets (elastic#83628) [Workplace Search] Initial rendering of Org Sources (elastic#84164) update geckodriver to 0.28 (elastic#84085) Fix timelion vis escapes single quotes (elastic#84196) [Security Solution] Fix incorrect time for dns histogram (elastic#83532) [DX] Bump TS version to v4.1 (elastic#83397) [Security Solution] Add endpoint policy revision number (elastic#83982) [Fleet] Integration Policies List view (elastic#83634) ...
Super late, but just wanted to say your approach in that commit looked good to me! 👍 |
Summary
This PR builds upon the previous PRs (#83954, #83799, #83544, #83459, #83799, #83593) to achieve a working render of the main Sources tree. The result is the majority of the Organization Sources section working as expected with more to be completed later.
Some notes:
ent-searchsidebar is just placed at the top of the main content area for now. Will work with design to get this fixed in a future PR./orgto determine context, that we would now flip it where we key of personal with/p.More extensive QA to be done after DisplaySettings and Schema are added and the server can handle the redirects correctly.
Best to follow along by commit with whitespace changes hidden