[Breadcrumbs] Remove private separator component

[ArcadeRenegade]

On initial page load, breadcrumbs are stacked on top of each other. This is causing the page to receive a "Clickable elements too close together" from Google Search Console.

When I load a different page, then navigate to the page with breadcrumbs (client side), then the breadcrumbs look fine. Seems like a JSS loading issue.

Any idea on how to prevent this from happening?

• [ x] The issue is present in the latest release.
• [ x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

On initial page load, breadcrumbs are stacked on top of each other.

Expected Behavior 🤔

Breadcrumbs look normally like this:

Steps to Reproduce 🕹

Have breadcrumbs with multiple list items on page, refresh page, breadcrumbs are stacked on top of each other.

Steps:

1. 2. 3. 4.

Context 🔦

Using:

• Gatsby v2.13.67
• Material UI Core 4.7.1
• Material UI Styles 4.7.1
• gatsby-plugin-material-ui 2.1.6

Your Environment 🌎

Tech	Version
Material-UI	v4.7.1
React	v16.9
Browser	Google Chrome
TypeScript
etc.

[oliviertassinari]

Do you have a reproduction?

[ArcadeRenegade]

Honestly, I tried really hard to recreate it in codesandbox but was not successful. Here is the codesandbox anyways:

https://codesandbox.io/s/condescending-http-qovdx

You can see it happen here on our website when you refresh the page:

https://www.roselium.com/product/olivia-bracelet/

[oliviertassinari]

Thanks for the link, this might help. I will have a look.

[ArcadeRenegade]

Thanks much. Let me know if there's anything I can help with.

[oliviertassinari]

Something is off with the rendering. In <style data-meta="PrivateBreadcrumbSeparator"> we have the class name under .jss65 in the style sheet while we have it under .jss63 on the DOM node.

@ArcadeRenegade Do you think that you could try a dichotomy to isolate the source of the problem? Remove half the rendering tree, see if the issue is present, iterate.

Alternatively, I don't think that we need this private BreadcrumbSeparator component, I think that we can kill it. I would propose this simplification:

diff --git a/packages/material-ui/src/Breadcrumbs/BreadcrumbSeparator.js b/packages/material-ui/src/Breadcrumbs/BreadcrumbSeparator.js
deleted file mode 100644
index 94f30067d..000000000
--- a/packages/material-ui/src/Breadcrumbs/BreadcrumbSeparator.js
+++ /dev/null
@@ -1,30 +0,0 @@
-import React from 'react';
-import PropTypes from 'prop-types';
-import clsx from 'clsx';
-import withStyles from '../styles/withStyles';
-
-const styles = {
-  root: {
-    display: 'flex',
-    userSelect: 'none',
-    marginLeft: 8,
-    marginRight: 8,
-  },
-};
-
-/**
- * @ignore - internal component.
- */
-function BreadcrumbSeparator(props) {
-  const { classes, className, ...other } = props;
-
-  return <li aria-hidden className={clsx(classes.root, className)} {...other} />;
-}
-
-BreadcrumbSeparator.propTypes = {
-  children: PropTypes.node.isRequired,
-  classes: PropTypes.object.isRequired,
-  className: PropTypes.string,
-};
-
-export default withStyles(styles, { name: 'PrivateBreadcrumbSeparator' })(BreadcrumbSeparator);
diff --git a/packages/material-ui/src/Breadcrumbs/Breadcrumbs.js b/packages/material-ui/src/Breadcrumbs/Breadcrumbs.js
index 18ea6a953..ca34dca35 100644
--- a/packages/material-ui/src/Breadcrumbs/Breadcrumbs.js
+++ b/packages/material-ui/src/Breadcrumbs/Breadcrumbs.js
@@ -5,7 +5,6 @@ import clsx from 'clsx';
 import withStyles from '../styles/withStyles';
 import Typography from '../Typography';
 import BreadcrumbCollapsed from './BreadcrumbCollapsed';
-import BreadcrumbSeparator from './BreadcrumbSeparator';

 export const styles = {
   /* Styles applied to the root element. */
@@ -23,7 +22,12 @@ export const styles = {
     listStyle: 'none',
   },
   /* Styles applied to the separator element. */
-  separator: {},
+  separator: {
+    display: 'flex',
+    userSelect: 'none',
+    marginLeft: 8,
+    marginRight: 8,
+  },
 };

 function insertSeparators(items, className, separator) {
@@ -31,9 +35,9 @@ function insertSeparators(items, className, separator) {
     if (index < items.length - 1) {
       acc = acc.concat(
         current,
-        <BreadcrumbSeparator key={`separator-${index}`} className={className}>
+        <li aria-hidden key={`separator-${index}`} className={className}>
           {separator}
-        </BreadcrumbSeparator>,
+        </li>,
       );
     } else {
       acc.push(current);

It would allow you to "hide" the problem, relying on the global class name but it won't solve the underlying issue, it might you hit you later down the road.

[ArcadeRenegade]

Yeah Gatsby does server side rendering then outputs a HTML file for every page. From what you describe there might be an inconsistency in the JSS between the server side rendered dom and the client side I think?

[oliviertassinari]

@ArcadeRenegade Yes, there is a difference between the rendering path on the server and on the client.

[ArcadeRenegade]

I ended up creating my own breadcrumb component since I wanted to support the breadcrumb structured data anyways. Should I leave this open or close it?

[oliviertassinari]

@ArcadeRenegade I think that we should keep it open. I think that it would help to kill the BreadcrumbSeparator component.

Regarding the structured breadcrumb, I assume you would like to use something like this:

<ol vocab="https://schema.org/" typeof="BreadcrumbList">
  <li property="itemListElement" typeof="ListItem">
    <a property="item" typeof="WebPage"
        href="https://example.com/books">
      <span property="name">Books</span></a>
    <meta property="position" content="1">
  </li>
  ›
  <li property="itemListElement" typeof="ListItem">
    <a property="item" typeof="WebPage"
        href="https://example.com/books/sciencefiction">
      <span property="name">Science Fiction</span></a>
    <meta property="position" content="2">
  </li>
  <li property="itemListElement" typeof="ListItem">
    <span property="name">Award Winners</span>
    <meta property="position" content="3">
  </li>
</ol>

I have personally always preferred the JSON-LD approach, but I can understand the need for the other approach. What do you think of two new props: olProps and liProps?

Alternatively, it seems that the following structure work just fine (doesn't need to change the API):

<nav vocab="https://schema.org/" typeof="BreadcrumbList">
<ol>
  <li>
    <div property="itemListElement" typeof="ListItem">
    <a property="item" typeof="WebPage"
        href="https://example.com/literature">
      <span property="name">Literature</span></a>
    <meta property="position" content="1">
    </div>
  </li>
  <li>
    <div property="itemListElement" typeof="ListItem">
    <span property="name">Speculative Fiction</span>
    <meta property="position" content="2">
    </div>
  </li>
  </ol>
</nav>

https://search.google.com/structured-data/testing-tool