updates notification drawer to support mobile #717

matthewcarleton · 2017-08-22T14:52:37Z

Description

The purpose of this is to add support for the notification drawer on vertical nav for mobile.

We will still need to address the issue with the jQuery where to drawer does not close when the hamburger menu is selected. This was fixed with Jeff's PR below.

This is related to this story:

https://patternfly.atlassian.net/browse/PTNFLY-1461

On smaller screens I've added a full height.
I removed the expansion option on the drawer on smaller screens.
In order to have the drawer sit above the menu on smaller screens I've introduced a unique z-index of 1035.

Link to rawgit and/or image

Vertical Example
https://rawgit.com/matthewcarleton/patternfly/notification-drawer-mobile-dist/dist/tests/notification-drawer-vertical-nav.html

Horizontal Example
https://rawgit.com/matthewcarleton/patternfly/notification-drawer-mobile-dist/dist/tests/notification-drawer-horizontal-nav.html

PR checklist (if relevant)

Responsive: works in extra small, small, medium and large view ports.
Preview the PR: An image or a URL for designer to preview this PR is provided.

…ion-drawer-mobile Close the notification drawer when showing the mobile vertical nav menu

jeff-phillips-18 · 2017-08-24T18:47:34Z

src/less/navbar-vertical.less

@@ -18,7 +18,7 @@
    position: fixed;
    right: 0;
    top: 0;
-    z-index: @zindex-navbar-fixed;
+    z-index: 1035;


This is a little concerning. Lots of the vertical-nav.less classes use @zindex-navbar-fixed as a base and add to that ( https://github.com/patternfly/patternfly/blob/master/src/less/vertical-nav.less#L293-L298 ). Adopters may also be using this to be able to show items over the navbar.

Is there sufficient reason to change this?

@jeff-phillips-18 we can revert it back now that we have the hamburger menu closing the drawer. This was changed to ensure the drawer sits about the open menu but that shouldn't be a problem.

@jeff-phillips-18 ok, the issue is now. If I change it back to 1030 then the menu can be open and if you open the drawer it's under the menu. Does it make sense to have the menu close if you open the drawer?

Yeah, it should close

jeff-phillips-18 · 2017-08-24T18:49:23Z

src/less/notifications-drawer.less

+      @media (max-width: @screen-xs-max) {
+        width:100%;
+        height: 100vh; // override to show use full height
+      }


Should the top value (58px) be subtracted?

@jeff-phillips-18 I'll add it in, it didn't see any visible difference when I had it but it can't hurt.

it is required for the drawer to expand/collapse correctly and still show the last group or actions/notifications if there is more than fit on the page.

jeff-phillips-18 · 2017-08-24T18:53:06Z

tests/pages/_includes/widgets/notification-drawer.html

@@ -1,6 +1,6 @@
 <div class="drawer-pf hide drawer-pf-notifications-non-clickable">
  <div class="drawer-pf-title">
-    <a class="drawer-pf-toggle-expand fa fa-angle-double-left"></a>
+    <a class="drawer-pf-toggle-expand fa fa-angle-double-left hidden-xs"></a>
    <a  class="drawer-pf-close pficon pficon-close"></a>


We don't show the expander at xs resolution but the drawer could still have the drawer-pf-expanded class (if expanded prior to shrinking the window). Is this something we should handle?

@jeff-phillips-18 good question. How concerned are we with these types of scenarios. They seem unlikely to me.

I'm OK with leaving it out. The JS for this is not shipped as an available function anyhow. We'll just need to remember to handle this in the JS repos.

with the change I just pushed I added in a media query to remove the left:270; that is added when expanded. It now will just take up full width. I think this is a good solution, what do you think?

matthewcarleton · 2017-08-25T14:28:47Z

@jeff-phillips-18 I've updated this to reflect the changes requested in patternfly/angular-patternfly#576 by @benjaminapetersen. Also added the js provided.

benjaminapetersen · 2017-08-25T15:21:52Z

Relevant to web console: openshift/origin-web-console#1326

benjaminapetersen · 2017-08-25T15:43:32Z

src/js/patternfly-functions-vertical-nav.js

@@ -3,7 +3,7 @@
 (function ($) {
  'use strict';

-  $.fn.setupVerticalNavigation = function (handleItemSelections) {
+  $.fn.setupVerticalNavigation = function (handleItemSelections, ignoreDrawer) {


Do you follow a convention when adding methods to the jQuery.prototype? For example, I've seen elsewhere pfDonutTooltipContents, pfPieTooltipContents, etc.

I realize you didn't create the function in this PR & are just modifying, but wanted to point this out just in case.

benjaminapetersen · 2017-08-25T15:46:21Z

src/js/patternfly-functions-vertical-nav.js

@@ -259,6 +261,15 @@
              // Always start at the primary menu
              updateMobileMenu();
              navElement.addClass('show-mobile-nav');
+
+              // If the notification drawer is shown, hide it


I'd suggest breaking this out into a handleDrawer() kind of function, rather than adding complexity to bindMenuBehavior, but totally up to your team style guidelines.

benjaminapetersen · 2017-08-25T15:50:33Z

src/js/patternfly-functions-vertical-nav.js

+                  $('.drawer-pf-trigger').removeClass('open');
+                  $drawer.addClass('hide');
+                }
+              }
            }
          } else if (navElement.hasClass('collapsed')) {
            window.localStorage.setItem('patternfly-navigation-primary', 'expanded');


I wonder if sessionStorage would be better for things like patternfly-navigation-primary. Is it essential to keep this information between browser restarts? I would tend to expect it to return to default collapse state between.

Also not part of your PR so feel free to ignore :)

This was done specifically for an application that needs this state maintained between restarts.

Hmm. possible to consider making these kinds of things configurable so that the needs of one app don't have to affect another?

Sure, this seems separate from this PR though. Please file an issue and consider contributing a PR.

Yup, not relevant for this PR. Will do!

benjaminapetersen · 2017-08-25T15:55:28Z

src/less/notifications-drawer.less

@@ -42,7 +42,9 @@
    display: block;
  }
  &.drawer-pf-expanded {
-    left: 270px;
+    @media (min-width: @screen-xs-max) {


Should it go full screen on mobile rather than partial? Orig comment: @bean66

Also bringing my gif back, If I expand the drawer, then shrink the page down, the drawer will go off screen (and stay off screen) as I click the toggle:

Reference screenshot re: should it go full width on mobile:

You are looking at the horizontal example which hasn't been fixed yet, this PR started out as just for the vertical example. I am working on the horizontal and will push shortly.

matthewcarleton · 2017-08-25T18:30:11Z

@jeff-phillips-18 @benjaminapetersen
I've updated this to support the horizontal menu as well. I'm wondering if showing the notification icon while showing the drawer on mobile makes sense. It will offer context when screen is sized down as well as show update when all notifications are read.

benjaminapetersen · 2017-08-25T19:58:04Z

src/less/notifications-drawer.less

+    @media (max-width: @screen-xs-max) {
+      width:100%;
+      height: calc(~"100vh - 58px");
+      top:34px; // this pushed the drawer below the notification icon in the menu


Curious if the top: 34px could be a variable that we could build off of. I know we have had conversations in the web console about having things pile up in that top left corner, once you open the drawer & have a toast or two... @rhamilto thoughts?

srambach · 2017-08-25T20:23:04Z

src/less/notifications-drawer.less

+
+      @media (max-width: @screen-xs-max) {
+        width:100%;
+        height: calc(~"100vh - 58px");


I'd love to see the px measurements in variables, especially since they are used multiple times and height relates to top.

Yup, esp if there is relationships between these values across components.

rhamilto · 2017-08-25T20:23:32Z

Curious if the top: 34px could be a variable that we could build off of. I know we have had conversations in the web console about having things pile up in that top left corner, once you open the drawer & have a toast or two... @rhamilto thoughts?

I think all of these positioning values should be variables so they can easily be overridden downstream. Otherwise entire rules have to be duplicated...

matthewcarleton · 2017-08-25T23:37:03Z

@srambach @benjaminapetersen @rhamilto That makes a lot of sense. @LHinson should we pull @andresgalante into this when he is back?

LHinson · 2017-08-28T19:32:33Z

Thanks @srambach @rhamilto @benjaminapetersen for the reviews. After syncing with @matthewcarleton, we are in agreement with your recommendation to make the position valuables variables.

I've filed two new issues for this. One for the notification drawer #725 and the other for the toast notifications #726.

Considering this update out of scope for this particular PR however will address that update next.

benjaminapetersen · 2017-08-29T14:18:03Z

src/less/navbar-vertical.less

@@ -46,7 +46,7 @@
          color: @navbar-pf-vertical-active-color;
        }
      }


Quick nit, you prob could eliminate this file from the PR as its just a whitespace change, or make sure you are using the patternfly/~.editorconfig. If everyone contributing is using it little whitespace shuffles like this should disappear.

benjaminapetersen · 2017-08-29T14:43:52Z

Pulled these updates into my local running origin-web-console. Noticed the following issue:

I believe you are switching to a mobile layout too soon:

So it seems 1,2 or 3 columns of content should work (the beauty of using flex), with varying lengths of text. I'd definitely like to see the date/time block collapse a bit as size decreases. There is a lot of opportunity to reclaim lost whitespace.

matthewcarleton · 2017-08-29T16:15:53Z

@benjaminapetersen curious to see how this is built. Can you show me the markup for one of the notifications?

spadgett · 2017-08-29T16:23:24Z

@matthewcarleton We're using angular-patternfly, which is handling the markup.

@jeff-phillips-18 Are any changes are needed downstream in angular-patternfly for this PR?

jeff-phillips-18 · 2017-08-29T16:33:48Z

@spadgett I think it is the notification body include that Matt is interested in. Angular Patternfly does not set the html layout of the notification body.

spadgett · 2017-08-29T16:55:04Z

Ah right, understood. Here is the markup structure:

https://github.com/openshift/origin-web-console/blob/master/app/views/directives/notifications/notification-body.html

matthewcarleton · 2017-08-29T17:18:10Z

@spadgett ah ok. So from what I can see you've changed the structure quite a bit and that's going to effect how the areas react at the breakpoint. I can create a quick html prototype to show what you'd need to get it to work if that is helpful.

benjaminapetersen · 2017-08-29T20:08:38Z

I am somewhat interested in the reason to move away from a container-fluid with rows/cols inside that use col-md*, col-sm*, etc to adjust the layout. Seems like that might be a good, generic but bootstrap consistent way to solve this?

matthewcarleton · 2017-08-30T12:14:26Z

@benjaminapetersen I wasn't here when that decision was made but I would assume it's taken from bootstrap itself. If you look at their components they don't use page layout within theirs either. It could be to avoid verbose code. @andresgalante would have some more insight on this I'm sure.

spadgett · 2017-08-30T12:28:16Z

@matthewcarleton What is the normal structure? We can try to align if that's the right thing to do and it accommodates the design.

This might break other downstream projects, though, since angular-patternfly allows any markup for the notification body. It seems like the CSS is assuming a lot about what the content is. Chaning flex-direction from row to column is significant :)

We've had similar problems in the past upgrading Patternfly with components like list-view.

LHinson · 2017-08-31T10:59:45Z

addresses comments seen in #713

matthewcarleton · 2017-08-31T13:58:42Z

@spadgett @benjaminapetersen have a look at the following: http://recordit.co/Lf1wsgXM4Y

I believe this is what you are trying to accomplish.

I've followed the example code here http://www.patternfly.org/pattern-library/communication/notification-drawer/#/code

I've added an additional
<span class="drawer-pf-notification-message">Your additional content here</span>
after the initial:
<span class="drawer-pf-notification-message">A New Event! Huzzah! Bold!</span>

This will allow you to make a simple change but stick to what patternfly suggests.

LHinson · 2017-08-31T15:15:06Z

Thanks @matthewcarleton for providing that.

Related to this issue and from a PF perspective, I think that an expanded view of the notification drawer (with additional content) is a new requirement for PatternFly and requires design review and docs.

Looping in @serenamarie125 as FYI who will help determine the priority for this. I've filed an issue against the patternfly-design repo regarding this issue to kick start a conversation: patternfly/patternfly-design#405

Since the above it out of scope, I'm going to merge this one and we will continue to follow up on the wizard as needed.

benjaminapetersen · 2017-09-01T19:50:54Z

Cross referencing some further updates @sg00dwin has done in the openshift web console to help improve readability with the type of content we are handling.

PR includes some discussion to upstream a few items.

openshift/origin-web-console#1997

updates notification drawer to support mobile

43ab41f

matthewcarleton requested review from andresgalante, jeff-phillips-18 and srambach August 22, 2017 14:52

jeff-phillips-18 and others added 2 commits August 22, 2017 15:51

Close the notification drawer when showing the mobile vertical nav menu

b0a88b0

Merge pull request #1 from jeff-phillips-18/matthewcarleton-notificat…

01f413e

…ion-drawer-mobile Close the notification drawer when showing the mobile vertical nav menu

jeff-phillips-18 reviewed Aug 24, 2017

View reviewed changes

matthewcarleton added 3 commits August 24, 2017 16:38

updates requested changes

e9e91f1

updates drawer to trigger close of menu

f0b4720

updates wrapping issue with notification drawer content on small screens

1c1eb47

jeff-phillips-18 mentioned this pull request Aug 25, 2017

Notifications drawer openshift/origin-web-console#1326

Merged

5 tasks

benjaminapetersen reviewed Aug 25, 2017

View reviewed changes

adds support for horizontal menu notification drawer

f792ff8

updates horizontal nav position on mobile

b32d014

benjaminapetersen reviewed Aug 25, 2017

View reviewed changes

removes comments

52d27a4

srambach reviewed Aug 25, 2017

View reviewed changes

benjaminapetersen mentioned this pull request Aug 28, 2017

Consider forms of configuration for app specific behavior #727

Closed

2 tasks

spadgett mentioned this pull request Aug 29, 2017

At smaller screens, the Drawer expand button actually causes it to shrink patternfly/angular-patternfly#577

Closed

jeff-phillips-18 approved these changes Aug 29, 2017

View reviewed changes

benjaminapetersen reviewed Aug 29, 2017

View reviewed changes

LHinson mentioned this pull request Aug 31, 2017

Notification Drawer: Expanded View for PatternFly patternfly/patternfly-design#405

Closed

2 tasks

LHinson merged commit e09fc17 into patternfly:master Aug 31, 2017

benjaminapetersen mentioned this pull request Sep 1, 2017

Drawer title appears to have fallen behind the accordion patternfly/angular-patternfly#607

Closed

matthewcarleton mentioned this pull request Sep 1, 2017

Date and time in the Drawer need to collapse at smaller screen sizes patternfly/angular-patternfly#576

Closed

LHinson mentioned this pull request Sep 7, 2017

At smaller screen sizes, notification drawer notification messages do not get enough space #721

Closed

updates notification drawer to support mobile #717

updates notification drawer to support mobile #717

Conversation

matthewcarleton commented Aug 22, 2017 • edited Loading

Description

Link to rawgit and/or image

PR checklist (if relevant)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewcarleton Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewcarleton commented Aug 25, 2017

benjaminapetersen commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewcarleton Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

matthewcarleton commented Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhamilto commented Aug 25, 2017

matthewcarleton commented Aug 25, 2017

LHinson commented Aug 28, 2017

Choose a reason for hiding this comment

benjaminapetersen commented Aug 29, 2017 • edited Loading

matthewcarleton commented Aug 29, 2017

spadgett commented Aug 29, 2017

jeff-phillips-18 commented Aug 29, 2017

spadgett commented Aug 29, 2017

matthewcarleton commented Aug 29, 2017

benjaminapetersen commented Aug 29, 2017

matthewcarleton commented Aug 30, 2017

spadgett commented Aug 30, 2017

LHinson commented Aug 31, 2017 • edited Loading

matthewcarleton commented Aug 31, 2017 • edited Loading

LHinson commented Aug 31, 2017

benjaminapetersen commented Sep 1, 2017

matthewcarleton commented Aug 22, 2017 •

edited

Loading

matthewcarleton Aug 24, 2017 •

edited

Loading

matthewcarleton Aug 25, 2017 •

edited

Loading

matthewcarleton commented Aug 25, 2017 •

edited

Loading

benjaminapetersen commented Aug 29, 2017 •

edited

Loading

LHinson commented Aug 31, 2017 •

edited

Loading

matthewcarleton commented Aug 31, 2017 •

edited

Loading