Skip to content
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 Style Fixes, Part 4 #79136

Merged

Conversation

MichaelMarcialis
Copy link
Contributor

Summary

Hey, @cqliu1! Here's a fourth (and hopefully last 😄) styling pass for the the Kibana overview page, which addresses some concerns that @cchaos brought up in her review, and also some renaming and restructuring of the header and footer components that are now being shared between the home and overview page. Whenever you have a moment, take a peek and let me know if you have any questions/suggestions. Notable changes include:

  • Switched from using display: block to adding a wrapping div in order to prevent EuiFlexItem children from rendering as flex items.
  • Replaced all eui prefixed class selectors with new custom classes.
  • Used flush="left" prop and applied CSS margin overrides to achieve the desired flush="both" effect (until the prop updates that @cchaos recently made make their way into Kibana's version of EUI).
  • Consolidated and unified shared page header/footer components markup and styles.
  • Renamed overview_footer to page_footer.
  • Renamed overview_header to page_header.

Parent Issue: #61549

@MichaelMarcialis MichaelMarcialis added the REASSIGN from Team:Core UI Deprecated label for old Core UI team label Oct 1, 2020
@MichaelMarcialis MichaelMarcialis requested review from a team as code owners October 1, 2020 15:05
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core-ui (Team:Core UI)

@kibanamachine
Copy link
Contributor

kibanamachine commented Oct 1, 2020

💔 Build Failed

Failed CI Steps

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
kibanaReact 303 312 +9

async chunks size

id before after diff
home 421.6KB 415.3KB -6.3KB
kibanaOverview 31.4KB 30.7KB -694.0B
kibanaReact 389.0KB 389.0KB +24.0B
total -7.0KB

page load bundle size

id before after diff
kibanaOverview 43.0KB 40.4KB -2.5KB
kibanaReact 134.9KB 152.3KB +17.4KB
total +14.9KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MichaelMarcialis I'm unable to run this locally... that build error looks unrelated to your changes, perhaps this just needs a fresh merge of master?

@cqliu1 cqliu1 merged commit 55b469c into elastic:kibana-overview Oct 2, 2020
@MichaelMarcialis MichaelMarcialis deleted the kibana-overview-design-fix-4 branch October 2, 2020 18:01
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few changes I'd make to this PR before merging, but ultimately could be done in the feature branch.

@@ -51,56 +51,65 @@ export const OverviewFooter: FC<Props> = ({ path }) => {
defaultRoute === path ? (
<RedirectAppLinks application={application}>
<EuiButtonEmpty
iconType="home"
className="pageFooter__button"
Copy link
Contributor

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.

Comment on lines +99 to +101
className="pageFooter__button"
data-test-subj="allPlugins"
flush="left"
Copy link
Contributor

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.

* under the License.
*/

.pageHeader {
Copy link
Contributor

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.

@cchaos
Copy link
Contributor

cchaos commented Oct 5, 2020

Ha, this PR merged already, I'll move my comments to the feature branch.

cqliu1 added a commit that referenced this pull request Oct 6, 2020
* 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>
cqliu1 added a commit to cqliu1/kibana that referenced this pull request Oct 6, 2020
* 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
cqliu1 added a commit that referenced this pull request Oct 6, 2020
* [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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REASSIGN from Team:Core UI Deprecated label for old Core UI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants