Feat(breadcrumb): Implements Breadcrumb Component #819

marshmalien · 2018-10-16T18:45:10Z

Feature: #33

This PR handles:

Basic, single line breadcrumb
Basic, single line breadcrumb without a home link
Breadcrumb with the last list item styled as Page Title

patternfly-build · 2018-10-16T18:48:17Z

Deploy preview for pf-next ready!

Built with commit aedded2

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

LHinson · 2018-10-18T12:06:57Z

Thanks for contributing @marshmalien! I've tagged @matthewcarleton and @michael-coker to see if they can review.

kybaker · 2018-10-18T13:21:54Z

@marshmalien Looks great! In Firefox the text is not vertically aligned but I am sure that is the Overpass bug.

mcoker · 2018-10-18T21:55:40Z

src/patternfly/components/Breadcrumb/breadcrumb.scss

+  // Breadcrumb Divider
+  --pf-c-breadcrumb__divider--MarginRight: var(--pf-global--spacer--sm);
+  --pf-c-breadcrumb__divider--MarginLeft: var(--pf-global--spacer--sm);
+  --pf-c-breadcrumb__divider--FontFamily: "Font Awesome 5 Free";


Should we add a global var for the font awesome font-family so we can reuse that in other components that apply the fa styles directly to a pseudo element?

I concur :)

mcoker · 2018-10-18T21:57:58Z

src/patternfly/components/Breadcrumb/breadcrumb.scss

+
+  &::before {
+    margin-right: var(--pf-c-breadcrumb__divider--MarginRight);
+    margin-left: var(--pf-c-breadcrumb__divider--MarginLeft);


if you remove this and apply that as margin-right to .pf-c-breadcrumb__item:not(:last-child), then when the breadcrumbs wrap, there won't be extra space to the left of wrapped lines

mcoker · 2018-10-18T22:00:50Z

src/patternfly/components/Breadcrumb/breadcrumb.scss

+  color: var(--pf-c-breadcrumb__item--Color);
+  cursor: default;
+
+  &:first-child::before {


I think you can remove this and rewrite the selector below to :not(:first-child):not(.pf-m-title)::before

mcoker · 2018-10-18T22:06:55Z

src/patternfly/components/Breadcrumb/breadcrumb.scss

+.pf-c-breadcrumb__item.pf-m-active * {
+  color: var(--pf-c-breadcrumb__item--m-active--Color);
+  text-decoration: none;
+  cursor: default;


this feels a little weird to me. personally I think if we're going to treat it this way, maybe it shouldn't be a link? Or if it is, leave the cursor behavior to indicate it's a link and remove the text treatment?

Also, I think you could rewrite this stuff in .pf-c-breadcrumb__item.pf-m-active * as &,&:hover {color, text decoration, cursor }, and you could rewrite .pf-c-breadcrumb__item.pf-m-active as &.pf-m-active nested in .pf-c-breadcrumb__item - just an option, not necessarily a suggestion :)

mcoker · 2018-10-18T22:08:32Z

src/patternfly/components/Breadcrumb/breadcrumb.scss

+  }
+}
+
+.pf-c-breadcrumb__item.pf-m-title {


this could also be &.pf-m-title under .pf-c-breadcrumb__item

mcoker · 2018-10-18T22:14:27Z

src/patternfly/components/Breadcrumb/breadcrumb.scss

+  }
+}
+
+.pf-c-breadcrumb__item.pf-m-active * {


can you change .pf-m-active to pf-m-current? we typically reserve active for the :active state.

mcoker · 2018-10-18T22:15:49Z

src/patternfly/components/Breadcrumb/breadcrumb.scss

+  line-height: var(--pf-c-breadcrumb__link--LineHeight);
+  color: var(--pf-c-breadcrumb__link--Color);
+
+  &:hover {


can you add a .pf-m-hover class and group that with this selector? Also should :focus and :active match these styles?

mcoker · 2018-10-18T22:17:16Z

src/patternfly/components/Breadcrumb/breadcrumb.hbs

@@ -0,0 +1,6 @@
+<nav aria-label="breadcrumb" class="pf-c-breadcrumb{{#if breadcrumb--modifier}} {{breadcrumb--modifier}}{{/if}}"


would you mind moving the aria-label to the line below so it's own its own line?

mcoker · 2018-10-18T22:22:13Z

src/patternfly/components/Breadcrumb/breadcrumb.scss

+  --pf-c-breadcrumb__item--hover--Color: var(--pf-global--Color--300);
+
+  // Breadcrumb Link
+  --pf-c-breadcrumb__link--FontSize: var(--pf-global--FontSize--sm);


I think you can remove this. The font size should be 14px, which is what --pf-global--FontSize--sm is (converts to 0.875rem), but you're setting it on the parent, too, so this is 0.875rem of 0.875rem

mcoker · 2018-10-18T22:26:12Z

@marshmalien this looks great! I left some comments for review. Let me know if you have any questions :)

matthewcarleton · 2018-10-22T12:34:49Z

src/patternfly/components/Breadcrumb/breadcrumb-item.hbs

@@ -0,0 +1,9 @@
+<li class="pf-c-breadcrumb__item{{#if breadcrumb-item--modifier}} {{breadcrumb-item--modifier}}{{/if}}{{#if breadcrumb-item--current}} pf-m-active{{/if}}"


Can you remove the conditional for active and just add it as a modifier in the example.

matthewcarleton · 2018-10-22T12:35:39Z

src/patternfly/components/Breadcrumb/breadcrumb-link.hbs

@@ -0,0 +1,6 @@
+<a href="{{breadcrumb-link--href}}" class="pf-c-breadcrumb__link{{#if breadcrumb-link--modifier}} {{breadcrumb-link--modifier}}{{/if}}"


For the sake of simplified HB code it's ok to just have href="#" rather than setting it every time in the examples. @michael-coker what do you think?

Yep, agreed! I doubt we'll ever need to set this to anything but # in the workspace.

matthewcarleton · 2018-10-22T12:38:55Z

src/patternfly/components/Breadcrumb/breadcrumb.scss

+  --pf-c-breadcrumb__divider--MarginLeft: var(--pf-global--spacer--sm);
+  --pf-c-breadcrumb__divider--FontFamily: "Font Awesome 5 Free";
+  --pf-c-breadcrumb__divider--Color: var(--pf-global--Color--300);
+  --pf-c-breadcrumb__divider--Content: "\f105";


I wonder if we should have a global var for this as well? I think we are using it in other places right?

I think so, but since this is the only place using a FA string like this currently, I don't think it's necessary for this PR. I mentioned it in #830, maybe we can address whether we define global vars for FA icons when reviewing our existing setup, and update this component accordingly?

matthewcarleton · 2018-10-22T12:41:12Z

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

+| `.pf-c-breadcrumb__item`        | `<li>`       | Initiates default breadcrumb list item |
+| `.pf-c-breadcrumb__link`        | `<a>`       | Initiates default breadcrumb list link |
+| `.pf-m-active`        | `.pf-c-breadcrumb__item`       | Modifies to display the list item as active |
+| `.pf-m-title`        | `.pf-c-breadcrumb__item`       | Modifies to display the display the list item as a page title |


Can we re-word this to "Modifies the last breadcrumb item to display as page title."

matthewcarleton · 2018-10-22T12:42:48Z

src/patternfly/components/Breadcrumb/examples/breadcrumb-example.hbs

@@ -0,0 +1,24 @@
+{{#> breadcrumb breadcrumb--modifier="pf-u-my-md"}}


I'm looking at the design and it has padding 32px top/bottom. Can we drop this utility for margin and just update the padding? This would apply to all examples :)

matthewcarleton · 2018-10-22T12:52:54Z

src/patternfly/components/Breadcrumb/examples/breadcrumb-with-title-example.hbs

+    {{/breadcrumb-item}}
+    {{#> breadcrumb-item breadcrumb-item--current="true"
+      breadcrumb-item--modifier="pf-m-title"
+      breadcrumb-item--attribute='title="Main Title"'}}


I ran this through VO and I'm not sure it's necessary. It announces the heading directly after the title text is announced so it seems redundant. Was this needed for JAWS or NVDA?

matthewcarleton · 2018-10-22T12:55:02Z

src/patternfly/components/Breadcrumb/examples/index.js

+      <Example heading="Breadcrumb" handlebars={breadcrumbExampleRaw} minHeight="10em">
+        {breadcrumbExample}
+      </Example>
+      <Example heading="Breadcrumb without Home Link" handlebars={breadcrumbWithoutHomeLinkRaw} minHeight="10em">


You can drop the minHeight on the first two examples, it's only needed if the content is overflowing the preview box.

matthewcarleton · 2018-10-22T12:57:39Z

src/patternfly/components/Nav/nav.scss

@@ -186,6 +186,16 @@
  &::-webkit-scrollbar {
    display: none;
  }
+
+  // Disable underline


is this meant to be part of this PR? I'd suggest making note of it if it is just to avoid confusion because it's part of a different component.

Yes, I meant to roll that change into this PR. Talked to @andresgalante and moved these selectors under .nav to prevent a global text-decoration.

matthewcarleton

Hey Marliana this is looking good! Just a few minor tweaks.

mcarrano · 2018-10-22T13:57:53Z

Just one small thing... In the first example it goes "Section Home > Section Title > Section Home > Section Landing." Shouldn't the third item be labeled "Section Title?"

* Add global text color $pf-global--Color--300 * Add example for default breadcrumb * Add example for breadcrumb with page title * Add breadcrumb documentation

* Change "active" modifier to "currrent" * Format code * Rewrite selectors

LHinson requested review from mcoker and matthewcarleton October 18, 2018 12:05

mcoker reviewed Oct 18, 2018

View reviewed changes

matthewcarleton reviewed Oct 22, 2018

View reviewed changes

mcoker mentioned this pull request Oct 23, 2018

Management of Font Awesome assets #830

Closed

matthewcarleton mentioned this pull request Oct 24, 2018

Update breadcrumb with requested changes #832

Closed

marshmalien added 3 commits October 24, 2018 10:47

Move global list and link selector under .pf-c-nav

41b13d9

Feat: Add breadcrumb css component

0ff0b49

* Add global text color $pf-global--Color--300 * Add example for default breadcrumb * Add example for breadcrumb with page title * Add breadcrumb documentation

Address review comments

aedded2

* Change "active" modifier to "currrent" * Format code * Rewrite selectors

mattnolting merged commit 40dd5d1 into patternfly:master Oct 25, 2018

LHinson mentioned this pull request Oct 29, 2018

Breadcrumbs #33

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat(breadcrumb): Implements Breadcrumb Component #819

Feat(breadcrumb): Implements Breadcrumb Component #819

marshmalien commented Oct 16, 2018

patternfly-build commented Oct 16, 2018 •

edited

LHinson commented Oct 18, 2018

kybaker commented Oct 18, 2018

mcoker Oct 18, 2018

matthewcarleton Oct 22, 2018

mcoker Oct 18, 2018

mcoker Oct 18, 2018 •

edited

mcoker Oct 18, 2018

mcoker Oct 18, 2018

mcoker Oct 18, 2018

mcoker Oct 18, 2018

mcoker Oct 18, 2018

mcoker Oct 18, 2018

mcoker commented Oct 18, 2018

matthewcarleton Oct 22, 2018

matthewcarleton Oct 22, 2018 •

edited

mcoker Oct 22, 2018

matthewcarleton Oct 22, 2018

mcoker Oct 23, 2018 •

edited

matthewcarleton Oct 22, 2018 •

edited

matthewcarleton Oct 22, 2018 •

edited

matthewcarleton Oct 22, 2018 •

edited

matthewcarleton Oct 22, 2018 •

edited

matthewcarleton Oct 22, 2018

marshmalien Oct 24, 2018

matthewcarleton Oct 24, 2018

matthewcarleton left a comment

mcarrano commented Oct 22, 2018

		@@ -0,0 +1,6 @@
		<nav aria-label="breadcrumb" class="pf-c-breadcrumb{{#if breadcrumb--modifier}} {{breadcrumb--modifier}}{{/if}}"

		@@ -0,0 +1,9 @@
		<li class="pf-c-breadcrumb__item{{#if breadcrumb-item--modifier}} {{breadcrumb-item--modifier}}{{/if}}{{#if breadcrumb-item--current}} pf-m-active{{/if}}"

		@@ -0,0 +1,6 @@
		<a href="{{breadcrumb-link--href}}" class="pf-c-breadcrumb__link{{#if breadcrumb-link--modifier}} {{breadcrumb-link--modifier}}{{/if}}"

		@@ -0,0 +1,24 @@
		{{#> breadcrumb breadcrumb--modifier="pf-u-my-md"}}

Feat(breadcrumb): Implements Breadcrumb Component #819

Feat(breadcrumb): Implements Breadcrumb Component #819

Conversation

marshmalien commented Oct 16, 2018

patternfly-build commented Oct 16, 2018 • edited

LHinson commented Oct 18, 2018

kybaker commented Oct 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcoker Oct 18, 2018 • edited

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

mcoker commented Oct 18, 2018

Choose a reason for hiding this comment

matthewcarleton Oct 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcoker Oct 23, 2018 • edited

Choose a reason for hiding this comment

matthewcarleton Oct 22, 2018 • edited

Choose a reason for hiding this comment

matthewcarleton Oct 22, 2018 • edited

Choose a reason for hiding this comment

matthewcarleton Oct 22, 2018 • edited

Choose a reason for hiding this comment

matthewcarleton Oct 22, 2018 • edited

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 left a comment

Choose a reason for hiding this comment

mcarrano commented Oct 22, 2018

patternfly-build commented Oct 16, 2018 •

edited

mcoker Oct 18, 2018 •

edited

matthewcarleton Oct 22, 2018 •

edited

mcoker Oct 23, 2018 •

edited

matthewcarleton Oct 22, 2018 •

edited

matthewcarleton Oct 22, 2018 •

edited

matthewcarleton Oct 22, 2018 •

edited

matthewcarleton Oct 22, 2018 •

edited