-
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
[serverless][poc] Unified Navigation #153274
[serverless][poc] Unified Navigation #153274
Conversation
Documentation preview: |
2f2164c
to
33b47b4
Compare
|
||
return ( | ||
<EuiThemeProvider colorMode={colorMode === 'DARK' ? 'LIGHT' : 'DARK'}> | ||
<EuiCollapsibleNav |
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.
In Security we have an ongoing initiative to display a collapsed version of the navigation, showing only icons. I am not sure if it is going to happen anytime soon, but that would be hard to implement if the core header owns the collapsible.
Would it make sense to delegate the collapsibility to the Project navigations? another option is to get a rendering element from Projects, instead of a children node, and pass the isOpen
by prop.
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.
a collapsed version of the navigation, showing only icons
This is @MichaelMarcialis and @ryankeairns vision for the navigation, so that would fall to Shared UX. We should try to surface these there, rather than force Security and other teams to implement themselves.
@tsullivan has been starting on a PoC for precisely this feature.
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.
Good! it looks like this is aligned with Security vision (cc @dimadavid @bfishel).
Sounds great to have a Shared UX nav component available, I hope we can all agree on the design and reuse it. But we shouldn't also force Solutions to use it.
Anyhow, in this navigation architecture, I understand that Solutions (in their serverless plugin) will be responsible for importing this new SharedUX component (or any other) and rendering it via setServerlessNavigation
, so it will still need to manage collapsibility, isn't it?
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.
Sounds great to have a Shared UX nav component available, I hope we can all agree on the design and reuse it. But we shouldn't also force Solutions to use it.
Agreed.
Anyhow, in this navigation architecture, I understand that Solutions (in their serverless plugin) will be responsible for importing this new SharedUX component (or any other) and rendering it via setServerlessNavigation, so it will still need to manage collapsibility, isn't it?
The collapsibility will already need to be there, because the items surrounding the solution "section" of the navigation will support the collapse behavior. There are still items to sort out, but yeah: setServerlessNavigation
currently appears to be the entire navigation, but as we flesh out other requirements, it will likely be one section, (e.g. Recent Items)
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.
I see, then this Solution nav "section" will also need to know if the nav is open or closed somehow, maybe we can do that with CSS only, not sure. Can't wait to see the Shared UX approach for the collapsed version.
Composing the nav with different sections sounds like a good pattern. It will probably limit the ability of Solutions to have their own navigation design (without becoming a "Franken navigation" 😂, just joking), but okay, we have to find a balance, let's see how it evolves 👍
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.
@semd I have this draft PR based on elastic:serverless/poc/unified_navigation
which adds a new package called @kbn/shared-ux-chrome-navigation
to implement a shared nav component: #153508
In my draft, the collapsibility is being handled by the ChromeService - there is a new internal observable to manage the open/closed state. Solutions can read the value of the observable just like any other observable, if they want to.
I'm eager to know what you think!
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.
@tsullivan Yeah looks good 👍 It is a smart solution. We'll be able to use the same observable from the Security navigation.
We lose the local storage functionality though, we have this feature in the internal Security nav for ESS. Would it make sense to implement it in the core internal nav? It has setNavIsOpen
available.
</EuiHeaderSectionItem> | ||
</EuiHeaderSection> | ||
</EuiHeader> | ||
<Router history={application.history}> |
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.
❤️
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 likely needs to change to core router in prod. @vadimkibana and @lukeelmers , can you weigh in?
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.
Not completely sure, would need to play with the code, but it might be already good. As the application.history
is the one from "core", so it sort of already is the "core router".
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.
I believe the history
here is pulling from core's internal application service contract, which means it is the global (unscoped) history instance that's currently only accessible to core.
We'll need to think about what we want to expose here for the project navs in serverless. Since the navs could be anything, it may be hard to avoid leaking the global history. cc @pgayvallet @rudolf
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.
The project nav isn't / shouldn't be scoped to a given plugin path, so using the global history seems fine to me, especially given its usage will be isolated in Core code / packages
a7af0ea
to
a0ccf6b
Compare
a0ccf6b
to
cc1a802
Compare
cc1a802
to
89bd09c
Compare
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
miscellaneous assets size
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
> Derived from #153274 for production. ## Summary This PR creates the `serverless` plugin for Kibana Serverless projects. ![image](https://user-images.githubusercontent.com/297604/233892935-b3713575-a2f7-4e82-a9dd-e8c11823683f.png) It uses the methodology proven out in the proof-of-concept (#153274) and prepares it for production: - Adds chrome style and related API to the `chrome` services. - Creates the `serverless` plugin. - Invokes the new chrome style API for all serverless projects. - Alters `yarn` scripts to support all project types, and switching between them. - Creates the new "Project Switcher" component for use in the new chrome header for Serverless. - Creates a Storybook config for this and future components. - Adds API endpoint to trigger project switching and `Watcher` restarts. <img width="1598" alt="Screenshot 2023-04-26 at 10 44 01 AM" src="https://user-images.githubusercontent.com/297604/234612654-fdcf38ea-8c48-4066-bc85-507f40c984aa.png"> ## Next steps - [x] Creating a PR for enabling/disabling related plugins for Serverless. (#155583) - [ ] Creating product plugin PR based on #153274. --------- Co-authored-by: kibanamachine <[email protected]>
> Derived from #153274 > Builds upon #155582 ## Summary This PR creates the Serverless Observability plugin, based on the work from #153274: - creates the plugin, - adds API to hide the solution navigation from Enterprise Search, - calls that API if the chrome style is `project`. <img width="1610" alt="Screenshot 2023-04-27 at 5 03 44 PM" src="https://user-images.githubusercontent.com/297604/234990765-d6770650-41b3-4e94-ad7f-c6a22778d39a.png"> --------- Co-authored-by: kibanamachine <[email protected]>
> Derived from #153274 > Builds upon #155582 ## Summary This PR creates the Serverless Security plugin, based on the work from #153274: - creates the plugin, - adds API to hide the solution navigation from Security, - calls that API if the chrome style is `project`. <img width="1688" alt="Screenshot 2023-04-27 at 12 37 46 PM" src="https://user-images.githubusercontent.com/297604/234979670-425bfb12-8194-4916-8f92-efff7804b577.png"> ## Next Steps - render the left nav from #153274 using an API provided by @elastic/appex-sharedux - this low-level API should be coming in the next few days. --------- Co-authored-by: kibanamachine <[email protected]>
Summary
This POC is an application of the principles I've outlined elsewhere, using "unified navigation" as a functional example, where:
core
changes are established as functional changes, rather than "serverless" changes;chromeStyle
,setSolutionNavigation
serverless
plugin provides APIs related to Serverless functional implementations;serverless-[project space]
plugins usecore
,serverless
and other plugins to create their Kibana configuration.By simply enabling the
serverless
andserverless-*
plugin, we can create a project.