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

[Proposal] Docking for wide viewports #130811

Closed
wants to merge 12 commits into from

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Apr 21, 2022

Relates to #77072

Summary

Prior versions of Kibana provided the capability to dock the primary navigation. This capability was removed when we introduced the solution-level side navigation as to avoid side-by-side navigation and, in turn, potentially cluttering up the UI.

Fast forward in time and we continue to see a steady stream of requests to add the dock option back, especially for cases where people use larger displays. Also, the width of the left nav design is now less than where it started. Digging into our telemetry data, we have also learned that the most common viewport size is 1920 x 1080. With this in mind, this PR proposes adding back the docking capability but only for wider viewport sizes where there are less concerns of available UI real estate.

Specifically, this PR allows the docking option for viewport widths of 1440px and up.

Screenshots

Discover < 1440
localhost_5601_gmt_app_discover (3)

Discover >= 1440
localhost_5601_gmt_app_discover (4)

Observability < 1440 (w/ solution side nav)
Screen Shot 2022-04-21 at 11 28 54 AM

Observability >= 1440 (w/ solution side nav)
Screen Shot 2022-04-21 at 11 28 17 AM

Checklist

Delete any items that are not applicable to this PR.

@ryankeairns
Copy link
Contributor Author

@elasticmachine merge upstream

@ryankeairns ryankeairns added auto-backport Deprecated - use backport:version if exact versions are needed ci:deploy-cloud labels Apr 21, 2022
@ryankeairns ryankeairns force-pushed the rk/dock-wide-screen branch from eb805eb to 1342c4d Compare April 21, 2022 22:15
@ryankeairns ryankeairns force-pushed the rk/dock-wide-screen branch from 827fb7f to 60c35bc Compare April 25, 2022 17:37
@ryankeairns
Copy link
Contributor Author

@elasticmachine merge upstream

@ryankeairns
Copy link
Contributor Author

@elasticmachine merge upstream

@ryankeairns ryankeairns marked this pull request as ready for review April 26, 2022 19:45
@ryankeairns ryankeairns requested review from a team as code owners April 26, 2022 19:45
@cchaos
Copy link
Contributor

cchaos commented Apr 26, 2022

I think this is a fair adjustment to allow larger screens to dock all navigation. The one thing we'll need to be concerned about are general EuiBottomBar's and especially the Security Timeline:
Screen Shot 2022-04-26 at 18 02 10 PM

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

Technically LGTM

@@ -24,3 +24,9 @@
text-decoration: underline; /* 2 */
}
}

@media (max-width: 1439px) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, but EUI may have constants and/or mixins around the most common break points? an hardcoded value probably should be avoided.

Comment on lines 371 to 373
if (lockRef.current) {
lockRef.current.focus();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: lockRef.current?.focus()

@ryankeairns
Copy link
Contributor Author

I'm going to need some help sorting out the space accommodations for views with a bottom bar before this can get merged.

I see there is something in the chrome_service for getting the docked state (getIsNavDrawerDocked$()), but I'm not certain how to wire that into the views where a bottom bar exists. Presumably, this boolean could be returned and used to set the left prop value on each EuiBottomBar bar instance (there are only a handful).

@ryankeairns
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 288.7KB 289.7KB +1021.0B

History

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

@FreerideTheWorld
Copy link

Finally. As per the feedback in [Discuss] Remove 'Dock navigation' feature #77072 this should have never been removed in the first place. What version will this be back in?

@tylersmalley tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022
@legrego legrego closed this Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed ci:cloud-deploy Create or update a Cloud deployment release_note:enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants