automatically style first and last breadcrumb

[gronke]

• The first breadcrumb item appeared to be off by -4px
• The last breadcrumb item does not require manual styling

[gronke]

The build is currently failing because of the usage of type selectors.

lerna ERR! execute lib/breadcrumb.scss
lerna ERR! execute  17:3  ✖  Expected "nav ol:last-child .breadcrumb-item::after" to have no more than 0 type selectors   selector-max-type
lerna ERR! execute  22:1  ✖  Expected "nav ol:first-child .breadcrumb-item" to have no more than 0 type selectors         selector-max-type

It might be possible to work with globs to work around the type selectors, but that's certainly not the most elegant solution if the breadcrumbs are supposed to be used in nav elements.

[shawnbot]

Hi @gronke, thanks for this. We try to avoid styling type selectors whenever possible, and we tend not to scope component styles with unrelated selectors, as in nav ol:last-child. There are utility classes that can achieve the same result. As written, this isn't an acceptable change for us, so I'm going to close this.

If you think that there's an issue with the existing styles, we welcome an issue with well-defined test cases. Cheers!

[shawnbot]

Hi there @gronke, sorry for closing this so abruptly. We'd like to incorporate your PR, but to scope it a bit more narrowly. Here's what would need to change:

1. Instead of removing the negative margin on only :first-child, we can remove this line:

   https://github.com/primer/primer-css/blob/6de1e89dd629c387e884775977fc78f84a3aea2a/modules/primer-breadcrumb/lib/breadcrumb.scss#L3

2. We prefer to be explicit about how selected styles are applied, so our recommended usage is to add both the breadcrumb-item and breadcrumb-item-selected classes to the last item. The :last-child styles should be removed.

Let me know if you're interested in making these updates, and we can reopen the PR. Thanks again!

[gronke]

Hi @shawnbot, that's absolutely fine - the changes I proposed broke with your code style rules. Do you want me to create an issue? Thank you for considering to re-open this PR, but I would not mind if you re-apply the changes in one of your commits.

1. Disabling the margin seems to change existing styles like here:

2. I was expecting the last item to always be selected and didn't see the need to leave it up to the user to define the style manually. The nav styles were only added to be more specific than the otherwise dominant normalization styles.

Btw. have you considered being explicit with styles rather than relying on a reset stylesheet? I am working on native web components built from primer-css and need to inject it into each custom elements ShadowDom.

[jonrohan]

Thanks for the contribution @gronke Sorry we went back and forth on it. We've decided to include it in our version 10 #354 Would you fix up the couple of things I suggested and we'll get this merged?

[jonrohan]

instead of this whole block can you remove the margin-left: -$spacer-1; line.

[jonrohan]

This is too specific here, instead will you drop the nav ol and just leave :last-child .breadcrumb-item

[gronke]

@jonrohan thanks for the review! I've addressed your comments and verified the output does not look broken on Chrome and Firefox.