-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Core UI] Kibana Overview Page #75827
Conversation
c055456
to
085e901
Compare
085e901
to
5350fb3
Compare
520d797
to
75d1f52
Compare
@ryankeairns @cqliu1 I'm not sure what the current state is exactly here, but we're starting to make progress on the Kibana analytics news feed |
7842407
to
e8bf950
Compare
This is looking really sharp. Only a few things to report on after an initial pass:
and a home page like: Admittedly, it's an edge case that a user would only have access to Visualize, so while I don't think we should invest too much effort into the design, we could probably tighten this up a bit. For example, some quick fixes might be:
@alexfrancoeur , et al, any other thoughts on the Viz-only experience? |
The page is looking great @cqliu1 ! Some quick feedback, thoughts and comments below.
|
I agree that Dev Tools and Settings don't seem like a fit and are likely carryovers from the home page header. In order to keep the header consistent between the no data and has data states, I would propose just removing those two and keeping Add data visible in both cases. As for the footer and the two links down there, I would keep it for a couple of reasons 1) for consistency but more importantly in my opinion 2) for visual reasons. Without the footer that blue button will be floating in space making the page feel unfinished. The footer visually frames the important content and, at least for me, makes the CTA more noticeable. |
I question whether we need to handle that config setting for this feed. Technically, this is a different feed and I don't believe the other solution overview pages hide their feeds based upon this. I could see us possibly providing a separate config option (e.g. |
Actually, |
Ah yeah, I forgot they went to separate places and agree that it is likely to be confusing having two similarly worded calls-to-action that go to different places. I'm good with only showing 'Add data' on the has data version. |
379d19c
to
301a373
Compare
I agree with the personal preference here, that works for me. That being said, I'm pretty sure the security solution uses this setting (based on this PR and earlier discussions). I'm not sure about observability, but it should. If it's too much work for the MVP that's fine, but we should support eventually (and probably check in on the observability feed).
If we are thinking about keeping these for a more appealing visual design, I'm wondering if we can make a few adjustments. While we all know I'd like to get rid of this app directory, it feels like it makes less sense on this page. The home page is the home page for all applications, I'm not sure how relevant it is here as the focus is on the Kibana apps. I'm OK with keeping this link for changing the home page, but maybe we can make it or action-specific to the Kibana homepage itself. "make this my homepage" kind of thing.
+1 |
I agree with @ryankeairns regarding dropping the add data, manage, and dev tools links from the pre-data landing page, but keeping the footer links. Am I correct in assuming that add data, manage, and dev tools will come back once data is present?
If possible, I'd ideally like to keep them and collect some telemetry on their usage. In addition to @ryankeairns' comments about consistency and framing, my hope was that all landing/overview pages across all solutions will share this same commonality, allowing users to quickly set a landing page as their new post-login route, or see the full breadth of applications that we offer. However, @alexfrancoeur, you're correct in noting that the currently implemented link to modify your post-login route is not correct. The original design intent was that when viewing a landing/overview page that is not currently your post-login route (such as this one), it should show a one-click option to make it your new post-login route. Only when the page is the current post-login route should we offer that generic link to modify your route settings. Examples can be found in the Figma mockups. @cqliu1, is this something we can easily change?
Having the smaller app cards appear partially below the fold doesn't bother me so much, as the use cases between the Kibana landing page and the Security and Observability overview pages are fairly different. In the Kibana landing page's case, the goal is more focused on what is possible and guiding users to the appropriate app for their needs (rather than delivering critical information as quickly as possible). As long as the page appears scrollable, I think we're OK. That said, if you strongly disagree and would prefer for the secondary solutions not to appear partially below the fold on smaller screens, a few quick possible come to mind that we can discuss:
Yes, this is a bug. I'll be cutting a PR between now and early next week that will address this. |
That's an interesting idea and would be simple way to get additional alignment with the other Overview pages. |
That is an interesting idea, is it too late to try out? A quick mock might be a good way to solidify the idea? We do want to start thinking about how this page could evolve as far as content goes (post-MVP), so it's possible it may be more than just additional navigation. |
I did some refactoring, and with the way it's implemented now, the overview page uses a service provided by the news feed plugin to retrieve the news feed, so it does support the Side note: If you set the i18n locale to anything besides English, the news feed won't show because the translations aren't provided by the news feed API. Is it okay that the news section will always be hidden for other languages, or do we want to show the articles in English regardless of locale? It looks like the global nav news is also empty, so I assume we just want this section hidden. |
f87b0e6
to
d750ec6
Compare
Created kivana_overview plugin Removed test from home plugin Added CSS Fixed page header links Added news feed Fixed spacers between news items [Core UI] Kibana Overview Page Style Tweaks (#76712) Fixed link to index management Added solution cards to kibana landing page Added solution links Fixed ts errors Using publishReplay() to support multiple consumers in newsfeed plugin Added createNewsFeed$ to newsfeed plugin start Added tests Removed unnecessary export Hides overview link when other Kibana apps are not available Added icon to overview plugin Removed question mark from news feed title Updated plugin-list Fixed i18n errors Revert snapshot Updated getting started page copy Hide news feed when no news feed results Disables Kibana overview page when kibana apps are unavailable Updated snapshots Refactor to use KibanaContextProvider Fixed security tests Fixed newsfeed api test Moved overview_footer and overview_header to kibana-react plugin [Core UI] Kibana Overview Page Style Fixes (#78677)
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.
Looks like app arch got pinged because of the PageHeader
and PageFooter
components which were added to the kibana_react
package. I just have one question on that:
Added PageFooter and PageHeader components to kibana-react plugin to allow other overview/landing pages to use the same header and footer shown on the home and Kibana overview pages
If these components exist only for overview/landing pages, I would vote for exporting them directly from the kibana_overview
plugin instead. This feels like the most logical domain for components that are for use in overview pages. (If you want to create your own overview page that looks similar to the global Kibana overview page, you'd expect to be able to find what you need in kibana_overview
).
If there's a reason we really need to keep them in kibana_react
, I would vote to name them something much more specific so folks understand their purpose in absence of a logical domain. (LandingPageHeader
? IDK)
These components are used in the |
I guess what I was suggesting is that the I don't feel so strongly that it's worth holding up the PR, however I do think we should consider renaming the components if they stay in Edit: The other minor benefit to putting the components 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.
I'd highly suggest reviewing some of the generic namings of the page header and footer.
Also, consider running all the SVG's through an optimizer (SVGO).
defaultRoute === path ? ( | ||
<RedirectAppLinks application={application}> | ||
<EuiButtonEmpty | ||
className="pageFooter__button" |
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.
All the pageFooter
classes need the kbn
prefix and ideally would be more specific even though the name of the component is simply PageFooter
because it's so general it could conflict in the future.
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 changed the prefixes to kbnOverviewPageFooter
and kbnOverviewPageHeader
<RedirectAppLinks application={application}> | ||
<EuiButtonEmpty | ||
className="pageFooter__button" | ||
flush="left" |
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 mentioned to @ryankeairns that the flush="both"
prop is now available in Kibana if this can be rebased with master.
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.
Awesome! I updated all of the buttons in the footer to flush="both"
|
||
return ( | ||
<header | ||
className={`pageHeader ${overlap ? 'pageHeader--hasOverlap' : 'pageHeader--noOverlap'}`} |
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.
Again all these classes ideally should be more specific and use the kbn
prefix.
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.
maps changes LGTM
code review
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'm comfortable with the naming updates to the kibana_react
components, and I'll defer to @cchaos's comments on the finer details of the implementation.
If we find that these components end up being frequently updated or are expanded significantly, then perhaps we can revisit the discussion of where they should live. But since the usage isn't widespread ATM, I'm okay with this as a starting point.
@cqliu1 It looks like the |
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.
Thanks for making those changes 👍
@cqliu1 That
|
Disregard. Your recent commits fixed this since they were shared. 🙌 |
💚 Build SucceededMetrics [docs]@kbn/optimizer bundle module count
async chunks size
distributable file count
page load bundle size
History
To update your PR or re-run it, just comment with: |
* Added kibana landing page Created kivana_overview plugin Removed test from home plugin Added CSS Fixed page header links Added news feed Fixed spacers between news items [Core UI] Kibana Overview Page Style Tweaks (elastic#76712) Fixed link to index management Added solution cards to kibana landing page Added solution links Fixed ts errors Using publishReplay() to support multiple consumers in newsfeed plugin Added createNewsFeed$ to newsfeed plugin start Added tests Removed unnecessary export Hides overview link when other Kibana apps are not available Added icon to overview plugin Removed question mark from news feed title Updated plugin-list Fixed i18n errors Revert snapshot Updated getting started page copy Hide news feed when no news feed results Disables Kibana overview page when kibana apps are unavailable Updated snapshots Refactor to use KibanaContextProvider Fixed security tests Fixed newsfeed api test Moved overview_footer and overview_header to kibana-react plugin [Core UI] Kibana Overview Page Style Fixes (elastic#78677) * Fixed a11y issues * Made newsfeed optional dep of kibana overview plugin * Removed duplicate license copy * Fixed management security test * Added toast to change default route button * Updated snapshots * Simplified toast notification * Fixed i18n error * Assigned kibana_overview plugin to Core UI in CODEOWNERS * Updated snapshots * Fix import * [Core UI] Kibana Overview Page Style Fixes, Part 3 (elastic#78970) * fix overview cards not stretching height equally * change var name for better specificity * [Core UI] Kibana Overview Page Style Fixes, Part 4 (elastic#79136) * Adds support for all newsfeed plugin config settings in createNewsFeed$ * Fixed type * Updated kibana overview page route * Fixed imports in page_footer and page_header * Update Kibana overview graphics (elastic#79534) * Updated snapshots * Updated snapshots * Changes newsfeed endpoint to kibana analytics in kibana_overview plugin * Renamed components * Fixed overview page footer and header component class names * Removed extraneous files * Fixed import * Replaced SVGs with optimized SVGs * Fixed header and footer in home and kibana overview pages * Updated snapshots * Changed url_forwarding plugin appRoute * Fixed aria-labelledby value * Updated snapshots * Added base paths Co-authored-by: Michael Marcialis <michael.marcialis@elastic.co> Co-authored-by: Ryan Keairns <contactryank@gmail.com> # Conflicts: # .github/CODEOWNERS
* [Core UI] Kibana Overview Page (#75827) * Added kibana landing page Created kivana_overview plugin Removed test from home plugin Added CSS Fixed page header links Added news feed Fixed spacers between news items [Core UI] Kibana Overview Page Style Tweaks (#76712) Fixed link to index management Added solution cards to kibana landing page Added solution links Fixed ts errors Using publishReplay() to support multiple consumers in newsfeed plugin Added createNewsFeed$ to newsfeed plugin start Added tests Removed unnecessary export Hides overview link when other Kibana apps are not available Added icon to overview plugin Removed question mark from news feed title Updated plugin-list Fixed i18n errors Revert snapshot Updated getting started page copy Hide news feed when no news feed results Disables Kibana overview page when kibana apps are unavailable Updated snapshots Refactor to use KibanaContextProvider Fixed security tests Fixed newsfeed api test Moved overview_footer and overview_header to kibana-react plugin [Core UI] Kibana Overview Page Style Fixes (#78677) * Fixed a11y issues * Made newsfeed optional dep of kibana overview plugin * Removed duplicate license copy * Fixed management security test * Added toast to change default route button * Updated snapshots * Simplified toast notification * Fixed i18n error * Assigned kibana_overview plugin to Core UI in CODEOWNERS * Updated snapshots * Fix import * [Core UI] Kibana Overview Page Style Fixes, Part 3 (#78970) * fix overview cards not stretching height equally * change var name for better specificity * [Core UI] Kibana Overview Page Style Fixes, Part 4 (#79136) * Adds support for all newsfeed plugin config settings in createNewsFeed$ * Fixed type * Updated kibana overview page route * Fixed imports in page_footer and page_header * Update Kibana overview graphics (#79534) * Updated snapshots * Updated snapshots * Changes newsfeed endpoint to kibana analytics in kibana_overview plugin * Renamed components * Fixed overview page footer and header component class names * Removed extraneous files * Fixed import * Replaced SVGs with optimized SVGs * Fixed header and footer in home and kibana overview pages * Updated snapshots * Changed url_forwarding plugin appRoute * Fixed aria-labelledby value * Updated snapshots * Added base paths Co-authored-by: Michael Marcialis <michael.marcialis@elastic.co> Co-authored-by: Ryan Keairns <contactryank@gmail.com> # Conflicts: # .github/CODEOWNERS * Updated kibana_overview application order Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
Blocked by #70571.Closes #61549.
Closes #76510.
This adds a getting started page and an overview page that highlights Kibana apps.
Getting Started (without index patterns)
All features available
OSS
Overview Page (With index patterns)
All features available
OSS
Notable changes
createNewsFeed$
inNewsfeed
plugin start to allow other plugins to subscribe to a specific Elastic Newsfeed API endpoint (see https://github.com/elastic/newsfeeds)PageFooter
andPageHeader
components tokibana-react
plugin to allow other overview/landing pages to use the same header and footer shown on the home and Kibana overview pagesChecklist
Delete any items that are not applicable to this PR.
For maintainers