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

[feature] category breadcrumbs #1949

Merged
merged 17 commits into from
Nov 12, 2019
Merged

[feature] category breadcrumbs #1949

merged 17 commits into from
Nov 12, 2019

Conversation

sirugh
Copy link
Contributor

@sirugh sirugh commented Oct 30, 2019

Description

  • Adds breadcrumbs to Category pages.
    Breadcrumbs endpoint, after 2.3.4, will return category_url_path which we can use to generate the link for the breadcrumb. For now, this is just rendering Links and dividers.

Related Issue

Closes PWA-98.

Acceptance

Verification Stakeholders

@schensley

Specification

Verification Steps

  1. From the landing page, click a category. You should see Home.
  2. From left nav, click all the way into a category (like Bottoms -> Skirts). It should render and you should see breadcrumbs for it.
  3. Do these things on desktop and mobile :)

Screenshots / Screen Captures (if appropriate)

Image from Gyazo

Checklist

  • I have updated the documentation accordingly, if necessary.
  • I have added tests to cover my changes, if necessary.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Oct 30, 2019

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PWA-98.

Generated by 🚫 dangerJS against 5b55b9f

@schensley
Copy link

@sirugh I think the implementation and behavior is good based on the fact that GraphQL cannot determine the specific path a shopper took to arrive at the PDP.

  1. I think the font size of the breadcrumbs should be reduced, while maintaining an adequate "tappable area".
  2. Add top padding to the breadcrumbs to provide more separation / clear space between the header and the breadcrumbs. Same for "Back" button on PDP.
  3. Could you explore center justification of the breadcrumbs, since the page title is centered? But keep the "Back" button (on PDPD) left-justified?

@brendanfalkowski
Copy link
Contributor

Consider dropping the back button? Users have a physical back button in their device, browser UI, or gestures.

Also probably a good idea to keep the breadcrumbs left aligned, and provide wrapping or overflow because IRL category names get long and they will overflow or wrap (sometimes so badly to the point that you decide to drop breadcrumbs).

@sirugh sirugh added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Oct 31, 2019
@sirugh
Copy link
Contributor Author

sirugh commented Oct 31, 2019

@schensley updates:

Image from Gyazo

@sirugh sirugh changed the title Initial breadcrumb implementation [feature] category breadcrumbs Oct 31, 2019
@sirugh sirugh marked this pull request as ready for review October 31, 2019 19:23
@zetlen
Copy link
Contributor

zetlen commented Oct 31, 2019

Hey @schensley, do you have an opinion on the @brendanfalkowski suggestion?

On the one hand, It's a best practice for us to keep the navigation obvious. Not all interactions are routable, so not everything works with a browser-based "Back" swipe.

But this one does! We don't have Back buttons everywhere. Should we omit this one, or put them everywhere?

One argument for explicit Back buttons is that a PWA can be installed to the Home screen. When that happens, the browser navigation goes away! The Back swipe still works (unless something has captured the gesture) but it's still a different runtime, so we shouldn't rely too much on that.

@sirugh
Copy link
Contributor Author

sirugh commented Oct 31, 2019

@zetlen I have since removed the Back button from the PDP as it was out of scope anyways. This PR is now just for breadcrumbs in the category page :)

@schensley
Copy link

schensley commented Nov 1, 2019

A few notes on the UI/UX as we work through this design.

  • Breadcrumbs should be left justified and extend from L to R in the viewport.

  • Breadcrumb list should terminate with the current page (non-link)

  • Need to better understand if/how GraphQL might allow for the structural category path to be determined and rendered on the PDP. This becomes valuable especially in cases where a shopper has arrived on the PDP from an external search.

  • Additionally, breadcrumbs should be allowed to wrap when the list exceeds the viewport width, without "line-breaking" a link.

@sirugh sirugh added the hold On hold until another condition is fulfilled. label Nov 1, 2019
@sirugh
Copy link
Contributor Author

sirugh commented Nov 1, 2019

hold as I need to address UX feedback. I will implement some form of breadcrumbs on the PDP as well -- it just may be a different look than whats on the category pages.

@sirugh
Copy link
Contributor Author

sirugh commented Nov 4, 2019

@schensley added non-functional text for current page to end of breadcrumbs. Also made it so the entire text wraps. See below pictures:
Image from Gyazo

Image from Gyazo

@sirugh sirugh removed the hold On hold until another condition is fulfilled. label Nov 5, 2019
@tjwiebell tjwiebell self-assigned this Nov 7, 2019
Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

Given your fondness for Toast, I think we should rename these to Croutons. /s

I think this looks great, just some minor tweaks so we're not hard-coding ourselves into .html routing (which is already present in existing components). Happy to chat about usages of useMemo too; I think there is a bigger cost to overusing than not, but that's up for debate.

@@ -43,6 +45,7 @@ const CategoryContent = props => {
const modal = filters ? <FilterModal filters={filters} /> : null;
return (
<Fragment>
<Breadcrumbs categoryId={categoryId} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Display breadcrumbs with this one simple prop!

@sirugh sirugh requested a review from tjwiebell November 8, 2019 21:25
tjwiebell
tjwiebell previously approved these changes Nov 8, 2019
Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

Approved, but would like your general thought about this design.

With moving the data fetch to the component, the re-render after data returns is very noticeable. Is this a state we should always handle? Common pattern is to display an indicator, but I'm foreseeing a future where a pages initial render is a ton of tiny loading indicators everywhere. The alternative of not rendering anything until data returns doesn't seem any better, and would cause layout push. What's the ideal solution here?

  1. Current State: Partial render (Home /) then re-paint with data
  2. ^^^ but with indicator until data
  3. No partial render, render indicator/skel until data
  4. Render nothing until data

@brendanfalkowski
Copy link
Contributor

I'd prefer rendering a skeleton until the data returns, so it doesn't shift.

Aside — we started developing breadcrumbs before this feature started, and made a couple different UX choices.

  1. Drop the "home" link. It's redundant because of the logo link, the obvious origin for all links, and takes up valuable space in the breadcrumb line.

  2. Don't show breadcrumbs when there's only one crumb. This means no breadcrumbs on top-level categories (because home is gone). So on sub-categories you'd see "Vaccuums > Robotic".

This ends up working a lot better on small screens, and doesn't get overloaded until three categories down.

@sirugh
Copy link
Contributor Author

sirugh commented Nov 8, 2019

@brendanfalkowski interesting. I ended up just implementing the design @schensley and I discussed but I think we could iterate on it. I don't think there's any rush to get this feature just yet so we may as well try to get it right the first time!

@sirugh
Copy link
Contributor Author

sirugh commented Nov 8, 2019

With moving the data fetch to the component, the re-render after data returns is very noticeable.

@tjwiebell Ah. I can return an empty, styled div so it's height is sized appropriately. It will still cause long (wrapping) crumbs to cause a pop-in though. We can't know the number of links or the length of the text so this is a difficult one. We could always show the last two crumbs in a link and just put some indicator of such before? ... / sub category / last category? Just throwing ideas around. Would love to hear from @schensley on these things :)

@brendanfalkowski
Copy link
Contributor

Another thing you can do to shorten the breadcrumb line is omitting the last crumb or putting a non-link (e.g. "here"). Whether it's a category, product, or CMS page that last crumb is usually redundant because the very next thing on screen is an H1 with the same text.

This makes a major difference on the product page because unlike categories product names can easily be 50+ characters alone if they have to distinguish variants.

@sirugh
Copy link
Contributor Author

sirugh commented Nov 12, 2019

Truncates based on width now. We may, in the future, want to drop the current category and the "Home" text but I'll let our UX engineers make that call.

Image from Gyazo

@dpatil-magento dpatil-magento merged commit ba25ec1 into develop Nov 12, 2019
@sirugh sirugh deleted the rugh/breadcrumbs branch March 4, 2020 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:peregrine pkg:venia-ui version: Minor This changeset includes functionality added in a backwards compatible manner.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants