Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

105832 update new pipeline job page #326

Merged
merged 8 commits into from
Aug 9, 2021

Conversation

Pespiri
Copy link
Contributor

@Pespiri Pespiri commented Aug 3, 2021

No description provided.

@Pespiri Pespiri requested a review from nilsgstrabo August 3, 2021 12:12
<NavLink className="breadcrumb__link" to={link.to}>
{link.label}
</NavLink>
<span className="breadcrumb__link-space">/</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend using ::after css to add "/" as separator between breadcrumb items. Combine this with :not(:last-child) to exclude "/" for the last element
The way it is now, if a breadcrumb item without [to] is inserted anywhere but at the end, it won't get the "/" separator.

CSS and component example:

.breadcrumb {
  font-size: 1.000rem;
}

.breadcrumb-list {
  display: inline-block;
}

.breadcrumb-list__item {
  display: inline-block;
}

:not(:last-child).breadcrumb-list__item::after {
  content: "/";
  margin: 0 var(--eds_spacing_medium) 0 var(--eds_spacing_medium);
}

.breadcrumb__text {
  color: var(--eds_text__static_icons__tertiary,rgba(111,111,111,1));
}

.breadcrumb__link {
  color: var(--eds_interactive_primary__resting);
}
import PropTypes from 'prop-types';
import React from 'react';
import { NavLink } from 'react-router-dom';

import './style.css';

const BreadcrumbLink = (link) => {
  if (link.to) {
    return (
      <>
        <NavLink className="breadcrumb__link" to={link.to}>
          {link.label}
        </NavLink>
      </>
    );
  }

  return <span className="breadcrumb__text">{link.label}</span>;
};

export const Breadcrumb = ({ links }) => {
  const linksRender = links.map((link, idx) => (
    <li className="breadcrumb-list__item" key={link.to || idx}>
      {BreadcrumbLink(link)}
    </li>
  ));

  return (
    <nav className="breadcrumb" role="navigation" aria-label="Page hierarchy">
      <ul className="breadcrumb-list">{linksRender}</ul>
    </nav>
  );
};

Breadcrumb.propTypes = {
  links: PropTypes.arrayOf(
    PropTypes.shape({
      label: PropTypes.node.isRequired,
      to: PropTypes.string,
    })
  ).isRequired,
};

export default Breadcrumb;

Copy link
Contributor Author

@Pespiri Pespiri Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have a look into it, but the '/' was removed from the .css to more closely resemble the EDS component until it has been fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The '/' not appearing after non-link breadcrumbs should now have been fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it is better to use ::after to insert the "/" instead of using a , the way it was before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the .css with your example with focus on the EDS styling

Copy link
Contributor

@nilsgstrabo nilsgstrabo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested changes on breadcrumb component

@Pespiri Pespiri force-pushed the 105832-update-New-Pipeline-job-page-2 branch from b687fb0 to a3ccaa8 Compare August 5, 2021 11:37
@Pespiri Pespiri force-pushed the 105832-update-New-Pipeline-job-page-2 branch from a3ccaa8 to fad934b Compare August 5, 2021 11:48
@Pespiri Pespiri requested a review from nilsgstrabo August 6, 2021 12:57
@Pespiri Pespiri merged commit b258881 into main Aug 9, 2021
@Pespiri Pespiri deleted the 105832-update-New-Pipeline-job-page-2 branch August 9, 2021 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants