feat(nav): add overflow styling to horizontal nav examples #1615

christiemolloy · 2019-03-22T13:52:22Z

fix #1519

Is it possible for the box shadow to change to white in the dark theme?

Visuals:

patternfly-build · 2019-03-22T13:55:28Z

Deploy preview for pf-next ready!

Built with commit 65ff9f3

https://deploy-preview-1615--pf-next.netlify.com

mcoker · 2019-03-22T14:44:46Z

Does this component's scroll buttons need the current styles, like used in tabs?

christiemolloy · 2019-03-22T18:29:45Z

@mcoker I don't think so as its just meant to be an addition to overflow scrolling but I can check

mcoker · 2019-03-22T18:37:59Z

Since the UI and basic interaction is the same, seems like they should match in that regard. Also, since this pattern is effectively the same between the components, I'm curious if this should be it's own component that nav and tabs use. And maybe it has a modifier to create both the UI used for the primary and secondary tabs examples.

christiemolloy · 2019-03-25T15:26:47Z

In terms of a new component would you be talking about the overflow button as a component or the entire horizontal list overflow. I don't know how I feel about that because one is a nav and one is a secondary tab list, and the lists have really different styles. and then we have the horizontal nav version for the masthead that seems like it should be an example of the nav component rather than using a component. I could also see the styles for secondary tabs maybe changing too @mcoker

mcoker · 2019-03-25T15:44:08Z

Looks like it would be an overflow-scroll component that you put a list inside of, and it could apply to tabs, secondary tabs, horizontal navigation, or if you had another list somewhere you wanted to treat similarly. Just in general, it's the same structure, and if I came across this pattern in the nav and tabs, I would expect it to behave the same or at least very similarly. The visual differences could be handled in the choice of icon or modifiers applied. It just looks like there is a lot of copy and paste between the two currently. What I would want to avoid is having 2 unique implementations because there are slight design changes between them. And question the design differences anyways, and ask if, since they're very similar patterns, they should present themselves more similarly, too.

I don't think we need to worry about it for this PR, but something to think about in the future.

mcoker

oops, approved the wrong PR, please disregard :)

mcoker · 2019-03-25T20:51:18Z

src/patternfly/components/Nav/nav.scss

+  padding-right: var(--pf-c-nav__scroll-button--PaddingRight);
+  padding-left: var(--pf-c-nav__scroll-button--PaddingLeft);
+  background-color: var(--pf-c-nav__scroll-button--BackgroundColor);
+  border: initial;


Is there a reason you use initial over 0?

This was used in the tab example so I thought it was purposeful. I can make a note to update both.

christiemolloy · 2019-03-26T15:03:21Z

@mcoker the reason I moved overflow-x: auto to the horizontal/tertiary list elements is because on the nav component itself, it was causing the buttons to scroll with the list. But placing it on the list fixes this.
Also something strange is happening in Firefox: the button isn't positioning absolute to its parent, its extending a certain amount below (hard to see), I don't think its because we're hiding the scrollbar with width: none, because this hasn't been a problem before. (this is also with top: 0, bottom: 0)

mcoker · 2019-03-26T19:03:10Z

Just want to verify that the right-side scroll button should extend to the edge of the viewport like this?

Also when I place the horizontal navigation in the page header nav, I can't scroll and the scroll button isn't visible.

mcoker · 2019-03-26T19:05:44Z

And this is how tertiary nav looks with both scroll buttons visible.

christiemolloy · 2019-03-26T20:37:45Z

--pf-c-page__main-nav--PaddingRight: 0 is assigned to 0 for some reason

christiemolloy · 2019-03-27T15:14:15Z

@mcoker updated this, visuals have approved of the box shadows.
Two things:

The box shadow for light and dark are hard coded. I didn't make them global vars because I think its a one off (i.e. we dont have a var for a white box-shadow or one that is the same color as dark mode), but that means the light/dark theming doesn't apply here and I had to add the dark var to the component...which I dont think we want.
Also I added a media query to the tertiary-list-item so that the first child has a padding-left of 32px on desktop, and then a padding-left of 16px on mobile. This looks really good, but I don't think we want to handle the padding in the component, and it would definitely be better for .pf-c-page-main-nav to handle the padding....so maybe we do explore negative positioning on the button?

I also added some styles to the page to show how the nav would work.. Ill remove those, but definitely check out the nav on the page component.

christiemolloy · 2019-04-02T18:16:10Z

@mcoker I successfully added the horizontal nav to the masthead in the page demo & updated the box shadow color as per @mceledonia recommendation

mcoker · 2019-04-02T20:56:50Z

src/patternfly/components/Nav/docs/code.md

@@ -24,6 +24,27 @@ The navigation system relies on several different sub-components:

 | Class | Applied To | Outcome |
 | -- | -- | -- |
+<<<<<<< HEAD


Looks like you have some merge remnants left in here...

mcoker · 2019-04-02T21:00:07Z

src/patternfly/components/Nav/nav.scss


-  // Hide scrollbars
-  &::-webkit-scrollbar {
+  &.pf-m-dark {


I wonder if instead of pf-m-dark, you should just style the button box-shadow in the horizontal nav? And can the color and background colors not inherit from @include pf-t-dark; in .pf-c-nav__horizontal-list?

mcoker · 2019-04-02T21:03:44Z

src/patternfly/components/Page/page.scss

@@ -204,6 +215,10 @@
  overflow: hidden;
  background-color: var(--pf-c-page__header-nav--BackgroundColor);

+  > * {


is there a reason you're using * instead of .pf-c-nav?

I was thinking for flexibility but I don't think that's necessary here so good point!

mcoker · 2019-04-02T21:07:24Z

src/patternfly/demos/Page/page-template-title.hbs

@@ -1,3 +1,38 @@
+{{#> page-main-nav page-main-section--modifier="pf-m-light"}}


Are you sure you want to add tertiary nav to every demo? Also the expandable nav already had tertiary nav, so now you have 2 in that demo - https://deploy-preview-1615--pf-next.netlify.com/demos/Page/examples-full/?component=Page%20Component%20Expandable%20Nav%20Example

mcoker · 2019-04-03T22:09:58Z

src/patternfly/components/Nav/nav.scss

  // Base styling
-  overflow-x: auto;


Sorry if I asked you this already, but why move this from nav to the horizontal nav components? Curious if this is necessary?

mcoker · 2019-04-03T22:13:59Z

@christiemolloy this looks great! Just one question about moving the overflow property from the main nav component into the horizontal nav variations.

Also, I see you are not using pf-m/t-dark, or including %pf-t-dark anywhere. Is the only remaining issue with the examples? If so, you can update the example component for those 2 examples to also include the pf-t-dark class.

      <Example
        heading="Horizontal Nav (only in masthead)"
        handlebars={navHorizontalListExampleRaw}
        className="is-dark-preview pf-t-dark"
      >
        {navHorizontalListExample}
      </Example>
      <Example
        heading="Horizontal Nav Overflow (only in masthead)"
        handlebars={navHorizontalListOverflowExampleRaw}
        className="is-dark-preview pf-t-dark"
      >

mcoker · 2019-04-03T23:04:37Z

Ah, I see what you're saying about needing a dark class on the nav if the buttons are outside of the list. In this case, we style another nav element based on the page element it's applied in, and I think that's fine here, too. The buttons have to be outside of the list though, otherwise you get a11y errors. Plus if we make this a component in the future, the buttons will need to be outside of the list (and probably the entire nav element).

In this PR - christiemolloy/patternfly-next#13 I refactored that and fixed a couple of a11y issues, as well as used aria-label="Global" on the main nav components in the demos, to match what's on react and what was on master before this PR. I'm assuming that was lost in a merge conflict? I'm not sure about whether to use aria-label="Global" or aria-label="Local" on the tertiary nav in the demos though. The tertiary nav in the current page demo on master uses "Global" but the tertiary nav in the Nav component examples uses "Local". Maybe @jgiardino can guide us here?

refactor(nav): refactor header-nav scroll buttons, a11y issues

christiemolloy · 2019-04-04T14:45:51Z

@mcoker thanks for making those changes! This is happening in the example because the page__header-nav has 'width: auto` on it rather than width: 100%, but it works in the demo, so not sure if it even matters:

mcoker · 2019-04-04T15:26:40Z

@christiemolloy I think that's fine. This is one of those cases where the example doesn't look right because it's outside of the context where it is designed to be used. Though looks like you could add .pf-c-page__header as a wrapper to those examples and that fixes it.

jgiardino · 2019-04-04T15:26:46Z

@mcoker I'm not seeing what you're describing.

We're using what's called Horizontal Nav not Tertiary Nav in our Page component. And on the the Nav component page, it does have aria-label="Global", which is correct.

I did notice that before this PR, the page component has two aria-label attributes though. One of them was changed to "Global" in this PR, so now at least they're both correct, but we only need one. 😉

mcoker · 2019-04-04T15:30:13Z

@jgiardino we use tertiary nav on the expandable nav page demo.

It's also in all of the data list demos and data table demos

jgiardino · 2019-04-04T15:29:05Z

src/patternfly/demos/DataList/data-list-page-nav.hbs

@@ -1,4 +1,4 @@
-{{#> nav nav--attribute=(concat 'id="' page--id '-primary-nav" aria-label="Grouped Nav Example"')}}
+{{#> nav nav--attribute=(concat 'id="' page--id '-primary-nav" aria-label="Global"')}}


aria-label appears to already be defined on the nav handlebar. It's showing up twice in the Page and Datalist demos.

Nice find! Looks like we should just remove any reference to aria-label from all of the nav components in the examples, except for the tertiary nav used in the page expandable demo, data list demo, and data table demos. And in those cases, define aria-label as an attribute of the hbs partial, not in the nav--attribute attribute value, since this is the logic that generates aria-label in nav.hbs

<nav class="pf-c-nav{{#if nav--modifier}} {{nav--modifier}}{{/if}}" {{#if aria-label}} aria-label="{{aria-label}}" {{else}} aria-label="Global" {{/if}} {{#if nav--attribute}} {{{nav--attribute}}} {{/if}}> {{> @partial-block}} </nav>

Or alternatively, we could remove the aria-label logic from nav.hbs, and include it in nav--attribute instead. That would probably be more consistent with our other partials.

jgiardino · 2019-04-04T15:38:47Z

we use tertiary nav on the expandable nav page demo.

Thanks for the example, @mcoker! That example uses the correct aria-labels. The primary nav is referred to as "global" navigation. Then the tertiary nav region is "local" navigation relative to the "Resource Usage" page that's selected in the global navigation.

mcoker · 2019-04-04T18:54:48Z

src/patternfly/demos/DataList/data-list-page-nav.hbs

@@ -1,4 +1,4 @@
-{{#> nav nav--attribute=(concat 'id="' page--id '-primary-nav" aria-label="Grouped Nav Example"')}}
+{{#> nav nav--attribute=(concat 'id="' page--id '-primary-nav" aria-label="Local"')}}


Suggested change

{{#> nav nav--attribute=(concat 'id="' page--id '-primary-nav" aria-label="Local"')}}

{{#> nav nav--attribute=(concat 'id="' page--id '-primary-nav" aria-label="Global"')}}

mcoker · 2019-04-04T18:59:57Z

Left one change suggestion.

Also. Can you just remove the nav component from this page component example, and replace the component with pf-c-nav text like the other examples?

https://github.com/patternfly/patternfly-next/blob/master/src/patternfly/components/Page/examples/page-component-nav-vertical-example.hbs#L17-L19

I also wonder if we should add aria-label to the nav component examples? It's on some of them but not all of them. We can create a follow up issue to do that, too.

mcoker

nice!!

patternfly-build · 2019-04-08T17:49:55Z

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

christiemolloy added 5 commits March 20, 2019 14:14

updates

776474d

updates

42f393f

update

48469f1

updates

584cf94

update

63468c2

christiemolloy requested a review from mcoker March 22, 2019 13:52

update

a257595

mcoker approved these changes Mar 25, 2019

View reviewed changes

christiemolloy added 2 commits March 25, 2019 16:00

updates

ef42b26

remove vars

10c8728

mcoker reviewed Mar 25, 2019

View reviewed changes

christiemolloy added 2 commits March 26, 2019 10:58

updates

7774019

update

d2a1fc2

christiemolloy added 2 commits March 26, 2019 13:09

fix bug

603b354

fix bug round2

9e34ae2

christiemolloy added 3 commits March 27, 2019 10:48

update styles for nav

983198c

show nav demo

49bab8b

change var name

f1dae2b

the second option for nav

69e8e85

mcoker reviewed Apr 2, 2019

View reviewed changes

christiemolloy added 3 commits April 3, 2019 11:34

updates

be29fff

update demos and update dark mode

8d73ada

updates but still broken

0f950e4

mcoker reviewed Apr 3, 2019

View reviewed changes

refactor(nav): refactor header-nav scroll buttons, a11y issues

5198a4a

Merge pull request #13 from mcoker/iss1519-2

6073398

refactor(nav): refactor header-nav scroll buttons, a11y issues

Merge branch 'master' into iss1519

0da48eb

add overflow nav to demos

7f213eb

jgiardino reviewed Apr 4, 2019

View reviewed changes

update aria-labels and examples

de9b3d9

mcoker reviewed Apr 4, 2019

View reviewed changes

remove aria labels from the components and fix them in demos

65ff9f3

mcoker approved these changes Apr 4, 2019

View reviewed changes

mcoker merged commit bfa521c into patternfly:master Apr 4, 2019

mcoker mentioned this pull request Apr 4, 2019

Implement overflow styles for horizontal navigation patternfly/patternfly-react#1709

Closed

patternfly-build added the released label Apr 8, 2019

		@@ -1,3 +1,38 @@
		{{#> page-main-nav page-main-section--modifier="pf-m-light"}}

		@@ -1,4 +1,4 @@
		{{#> nav nav--attribute=(concat 'id="' page--id '-primary-nav" aria-label="Grouped Nav Example"')}}
		{{#> nav nav--attribute=(concat 'id="' page--id '-primary-nav" aria-label="Global"')}}

feat(nav): add overflow styling to horizontal nav examples #1615

feat(nav): add overflow styling to horizontal nav examples #1615

Conversation

christiemolloy commented Mar 22, 2019 • edited

patternfly-build commented Mar 22, 2019 • edited

mcoker commented Mar 22, 2019

christiemolloy commented Mar 22, 2019

mcoker commented Mar 22, 2019

christiemolloy commented Mar 25, 2019

mcoker commented Mar 25, 2019

mcoker left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christiemolloy commented Mar 26, 2019 • edited

mcoker commented Mar 26, 2019

mcoker commented Mar 26, 2019

christiemolloy commented Mar 26, 2019

christiemolloy commented Mar 27, 2019

christiemolloy commented Apr 2, 2019

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

mcoker commented Apr 3, 2019

mcoker commented Apr 3, 2019

christiemolloy commented Apr 4, 2019

mcoker commented Apr 4, 2019

jgiardino commented Apr 4, 2019

mcoker commented Apr 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgiardino commented Apr 4, 2019

Choose a reason for hiding this comment

mcoker commented Apr 4, 2019

mcoker left a comment

Choose a reason for hiding this comment

patternfly-build commented Apr 8, 2019

christiemolloy commented Mar 22, 2019 •

edited

patternfly-build commented Mar 22, 2019 •

edited

mcoker left a comment •

edited

christiemolloy commented Mar 26, 2019 •

edited