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

full screen mode implementation for dashboard #12265

Merged
merged 12 commits into from
Jun 23, 2017

Conversation

stacey-gammon
Copy link
Contributor

@stacey-gammon stacey-gammon commented Jun 9, 2017

Fixes #9268

Release Note:
You can now enter full screen mode when viewing a dashboard. This hides the Chrome and the top nav bar. If you have any filters applied, you'll see the filter bar, otherwise that will be hidden as well. To exit full screen mode, hover over and click the Kibana button on the lower left side of the page, or simple press the ESC key.

screen shot 2017-07-25 at 3 51 15 pm

@stacey-gammon stacey-gammon added Feature:Dashboard Dashboard related features :Sharing labels Jun 9, 2017
@alexfrancoeur
Copy link

@stacey-gammon woo!! this PR is fantastic. I saw some comments in slack around showing / hiding the query bar and filter bar on hover and agree that should be in addressed in another PR. I also think that for the first iteration, we should address the most prominent use case of showcasing this dashboard. I can't imagine there will be much need for interaction and querying in full screen mode for this scenario. This is looking great! I have some usability comments below.

  • Is it possible to trigger the browser to go in full screen mode as well when clicking the full screen button? I expected my browser to expand to full screen as well when clicking on this button. Not the end of the world if we can't (progress over perfection) but this is common behavior amongst other dashboarding tools that have this feature

  • Can we add keyboard controls here for exiting full screen mode? I was aware of the button on the bottom but also attempted to use the esc key to leave full screen mode. Just a though. This may conflict with browser full screen modes as well. It'd be nice if hitting esc exited both browser and Kibana full screen mode.

  • I see full screen mode in edit mode. It seems odd to me to have that there. I assume this is necessary because when resizing Kibana we automatically expand the panels and not shift the around to fit the screen. Is that correct?

  • Somewhat related, I noticed that we persist the side bar space until you adjust the browser size
    jun-12-2017 10-41-02

  • It's a bit hard to read "Exit full screen mode" in dark mode. There isn't enough contrast. It'd be good to get @snide's feedback here

screen shot 2017-06-12 at 10 26 46 am

Hope this feedback helps! Let me know if you need any other clarification

@cjcenizal
Copy link
Contributor

This is REALLY COOL. This features makes dashboards feel much more polished, to an extent that is disproportionate to the size of the feature.

I noticed a UX bug. When the dashboard has no panels, clicking "Add" should probably pop you out of full-screen mode so you can see the "Add panel" dropdown:

image

I also couldn't get the filter and query bar to show up in full screen mode. Am I supposed to be able to do that in this version of the PR? If so, what do I need to do to get them to show?

That said, I like all of @alexfrancoeur's suggestions above.

My only suggestion would be about our approach. I suggest we refine the UX first and then tackle the appearance and behavior of the "Exit full screen mode" button as the very last step. I think we can make some adjustments to make it more consistent with the look-and-feel and I'm sure @snide has thoughts on this too.

@stacey-gammon
Copy link
Contributor Author

@alexfrancoeur -

Is it possible to trigger the browser to go in full screen mode as well when clicking the full screen button? I expected my browser to expand to full screen as well when clicking on this button. Not the end of the world if we can't (progress over perfection) but this is common behavior amongst other dashboarding tools that have this feature

It's possible (though sounds like it might be browser specific), but I'm not sure I agree with this one. I tried in Google Docs, and full screen doesn't automatically pop the browser into full screen mode. I can see a use case of someone wanting to maximize the browser space, but still be able to quickly tab to other browser windows/desktop icons/etc.

Can we add keyboard controls here for exiting full screen mode? I was aware of the button on the bottom but also attempted to use the esc key to leave full screen mode. Just a though. This may conflict with browser full screen modes as well. It'd be nice if hitting esc exited both browser and Kibana full screen mode.

Great idea, and actually checking out Google Docs full screen mode for the above, looks like they only offer a keyboard way to exit Full screen mode. An interesting idea if we want to get rid of the button on the bottom.
screen shot 2017-06-13 at 9 56 43 am

I see full screen mode in edit mode. It seems odd to me to have that there. I assume this is necessary because when resizing Kibana we automatically expand the panels and not shift the around to fit the screen. Is that correct?

I can get rid of it from full screen mode, but I could potentially see the use case, albeit much less common. Maybe just wanting to move around, resize, or see how it will look in Full Screen mode.

Somewhat related, I noticed that we persist the side bar space until you adjust the browser size

Will investigate

It's a bit hard to read "Exit full screen mode" in dark mode. There isn't enough contrast. It'd be good to get @snide's feedback here

Good catch, will look into.

@cjcenizal

I noticed a UX bug. When the dashboard has no panels, clicking "Add" should probably pop you out of full-screen mode so you can see the "Add panel" dropdown:

Ah good point, will fix.

I also couldn't get the filter and query bar to show up in full screen mode. Am I supposed to be able to do that in this version of the PR? If so, what do I need to do to get them to show?

Query bar doesn't show up, filter bar should show up if you have a filter applied.
screen shot 2017-06-13 at 11 58 15 am

We can investigate further options for when to show the query bar (perhaps offer some configurations - would be nice to do the same for embedded mode). But as a first pass, I think we should punt on it. The kbn top nav is tightly coupled to the chrome being visible, so the logic would have to be refactored out first. We'd also have to figure out the UI for the configuration. So I think those are great ideas, but I don't think it should hold up this more simplified version.

@stacey-gammon
Copy link
Contributor Author

Fixed the bugs mentioned, will wait to hear about overall styling on the button before fixing the dark theme version.

@stacey-gammon
Copy link
Contributor Author

jenkins, test this


.exitFullScreenMode:hover {
bottom: 0px;
transition: all .5s ease;
Copy link
Contributor

@snide snide Jun 20, 2017

Choose a reason for hiding this comment

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

Here's a cleaner animation for you...

Does the following

  • Moves the bug to the top, which is where the controls originally start.
  • Uses more reliable vars.
  • Improves the transition by
    • Using transform instead of position (much smoother, gpu now takes over).
    • Puts transition on the base selector, now works on on and off hover, instead of just on.
    • Speeds it up. In general, no hover animation should take longer than 250ms.
    • When in doubt use ease-in-out, rather than just ease.

Note that you should also up the padding to .dashboard-grid to 20px if you move it up. In general I've been adding similar increased padding across Kibana, so it's not just for this PR that we need it.

.exitFullScreenMode {
  background-color: @globalColorLightestGray;
  border-bottom-left-radius: 4px;
  border-bottom-right-radius: 4px;
  border-top-left-radius: 0;
  border-top-right-radius: 0;
  width: 200px;
  height: 40px;
  left: 0;
  right: 0;
  top: 0;
  margin-left: auto;
  margin-right: auto;
  padding-bottom: 0;
  position: fixed;
  transform: translateY(-26px);
  z-index: 50;
  display: block;
  transition: transform .2s ease-in-out;
  border: solid 1px @globalColorLightGray;

  .kuiIcon {
    display: block;
  }

  &:hover {
    transform: translateY(-1px);
  }
}

@stacey-gammon
Copy link
Contributor Author

Added the new styles (with some slight modifications, including leaving the button on the button after chatting with @snide about the way it looks on the top with filters applied).
screen shot 2017-06-20 at 4 26 18 pm
screen shot 2017-06-20 at 4 26 22 pm

Also addressed dark theme variation:
screen shot 2017-06-20 at 4 23 34 pm

And remove Full Screen option from edit mode because it's awkward with the lack of the obvious Save option. Can easily add it in later if we deem it appropriate.

Copy link
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

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

I think this functionality is great! Just a few comments, and I noticed that the browser back button doesn't play that nicely with exiting full screen mode.

@@ -25,6 +25,7 @@ import { notify } from 'ui/notify';
import './panel/get_object_loaders_for_dashboard';
import { documentationLinks } from 'ui/documentation_links/documentation_links';
import { showCloneModal } from './top_nav/show_clone_modal';
import { ESC_KEY_CODE } from 'ui_framework/services';
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@@ -68,9 +78,11 @@
</form>
</div>
</kbn-top-nav>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a dangling closing tag, and should be able to be removed.

$scope.fullScreenMode = fullScreenMode;
dashboardState.setFullScreenMode(fullScreenMode);
chrome.setVisible(!fullScreenMode);
$scope.$root.$broadcast('globalNav:update');
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that we should be emitting the globalNav:update event as this is something that the globalNav itself used to be doing, and we're hijacking it to re-render the dashboard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, I figured the global navigation did update, so it wasn't that bad, though I hate those broadcasts. I can do $window.dispatchEvent(new Event('resize')); to trigger the re-render of the grid, which should also fixe the bug @nreese discovered. How do you feel about that? I don't think there is really a good way to tell gridster to resize directly. Or maybe I'm missing a more obvious solution...

Copy link
Contributor

Choose a reason for hiding this comment

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

Using the native resize event feels equally gross because it's normally fired by the browser itself, and it feels like we'd be hijacking this as well.

Since the dashboard-grid is a directive within the dashboard we shouldn't have to resort to using a $scope.$root.$broadcast and should be able to just call a function that we pass to it if we need to trigger the resize.

chrome.setVisible(!fullScreenMode);
$scope.$root.$broadcast('globalNav:update');
if (fullScreenMode) {
onRouteChange = $scope.$on('$routeChangeStart', () => chrome.setVisible(true));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not following what we're doing here, we're calling chrome.setVisible above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a comment to make it clearer. I need to add this so that if the user navigates to a different app, or clicks the back button, the user is bounced out of full screen mode. Otherwise, you can end up with something like this:
screen shot 2017-06-21 at 2 33 57 pm

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. When the user navigates away, how is the onRouteChange/deregistration called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I think I just need to add a call to onRouteChange inside the $routeChangeStart handler. Seems to do the trick.

@@ -13,13 +58,12 @@
dashboard-grid {
display: block;
margin: 0;
padding: 5px;
padding: 20px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we mean to change this here? It seems to add quite a bit of padding even when not in full-screen mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I've increased the padding on a bunch of our inner panels (see monitoring...etc) to give the controls some space. This was actually one of the few places I missed in my big K6 PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's more accessible for those of us with fat fingers! 😉

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

Looks good. I just found one instance where an exception is thrown.

$scope.fullScreenMode = fullScreenMode;
dashboardState.setFullScreenMode(fullScreenMode);
chrome.setVisible(!fullScreenMode);
$scope.$root.$broadcast('globalNav:update');
Copy link
Contributor

Choose a reason for hiding this comment

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

$root can be null when you first open a dashboard, then set to full screen, then use esc to close

root_is_null

@stacey-gammon
Copy link
Contributor Author

Great call on the back button @kobelb, and nice find @nreese. Both issues should be fixed now!

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

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

LGTM

@stacey-gammon
Copy link
Contributor Author

jenkins, test this

@alexfrancoeur
Copy link

This looks great @stacey-gammon!

@stacey-gammon
Copy link
Contributor Author

jenkins, test this

@jimgoodwin
Copy link

@stacey-gammon can we get a Release Note: paragraph at the end of the description describing the functionality please...

@stacey-gammon
Copy link
Contributor Author

done @jimgoodwin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants