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

TCCP: Add breadcrumbs to card detail page #8263

Merged
merged 2 commits into from
Mar 26, 2024

Conversation

niqjohnson
Copy link
Member

Adds breadcrumbs to the card details page and progressively enhances them with JavaScript so the breadcrumb that points back to the card list will take you to the specific filtered list you came from.

We have to update that breadcrumb client-side because there's a chance Akamai may cache a version of the page with a breadcrumb to a specific filtered list if we do it on the backend, see #7108 for details. I'm comfortable making this enhancement here even though we chose to remove it in other places. A primary goal of the TCCP tool is to help users compare different cards that meet their needs, so we're actually hoping they'll go from a filtered list to card details back to the filtered list and so on to look at a number of different cards. Most people will use the back button for that, but for those who don't, we want to make sure they can always get back to the filtered list they came from.

@chosak and @contolini, a couple of specific questions I had with this one:

  1. Now that our TCCP JS is running on two pages and not just the card list page, should we scope what happens in init() so each page only runs the relevant stuff? The previousPage.pathname.includes(cardListPagePath) condition in updateBreadcrumb() should prevent the breadcrumb update from happening on the card list page, but is that better done elsewhere?
  2. Does the previousPage.hostname === window.location.hostname condition assuage our concerns about only updating the breadcrumb if you're coming from a cf.gov page?
  3. Do we need unit tests for this JS now that it's gotten a little more complex?

And more generally, any other approaches to this that would make more sense?


Additions

  • Breadcrumbs to the card details page

How to test this PR

  1. Fire up http://localhost:8000/consumer-tools/credit-cards/explore-cards/ and choose some situations
  2. From the card list page, choose a card to view; also note that the breadcrumbs on the card list page aren't affected
  3. Confirm the breadcrumb on the card details page goes back to the filtered view you came from
  4. Back on the card list page, change a filter and note that the URL doesn't change (yet, see the todo below)
  5. From that newly filtered list, choose another card to view
  6. Note the breadcrumb on that card details page still points to the original filtered list (again, see the todo below)
  7. Finally, copy and paste a card details URL into a new tab, and note that the breadcrumb points back to the unfiltered card list (since there's no referrer)

Screenshots

breadcrumbs

Notes and todos

  • I noted this in a TODO in the code, but right now we're not doing a replaceState when you change the filters on the card list page, so technically this breadcrumb isn't fully working yet. It will always take you back to the filtered list you first landed on from the landing page, even if you then change the filters on the list page. @contolini is working on a fix to enable replaceState to work with Akamai caching. When that's done we'll check the breadcrumbs to make sure they're working as expected (and remove the TODO note).

Checklist

  • PR has an informative and human-readable title
    • PR titles are used to generate the change log in releases; good ones make that easier to scan.
    • Consider prefixing, e.g., "Mega Menu: fix layout bug", or "Docs: Update Docker installation instructions".
  • Changes are limited to a single goal (no scope creep)
  • Code follows the standards laid out in the CFPB development guidelines
  • Future todos are captured in comments and/or tickets

@niqjohnson niqjohnson requested review from chosak and contolini March 21, 2024 21:36
@niqjohnson niqjohnson changed the title Add breadcrumbs to TCCP card detail page TCCP: Add breadcrumbs to card detail page Mar 22, 2024
@niqjohnson
Copy link
Member Author

Putting a hold on this one. It sounds like @contolini has a better way to handle breadcrumbs with HTMX, so this workaround my not be needed at all.

@contolini
Copy link
Member

Now that our TCCP JS is running on two pages and not just the card list page, should we scope what happens in init() so each page only runs the relevant stuff?

Great point. Due to the small size of the app, I think I'd prefer to keep everything in one file and add scoping logic that's specific to the functionality (instead of adding logic like "if on page X do Y"). So the updateBreadcrumb function might look like:

function updateBreadcrumb() {
    const crumb = document.querySelector('.m-breadcrumbs_crumb:last-child');
    if (crumb.innerText === 'Customize for your situation') {
        crumb.href = webStorageProxy.getItem('tccp-filter-path') || crumb.href;
    }
}

Do we need unit tests for this JS now that it's gotten a little more complex?

I say we lean on cypress tests for now (I added some in #8266) because so far all our JS is heavily tied to the DOM.

@niqjohnson niqjohnson force-pushed the tccp/card-details-breadcrumbs branch from a4c8576 to 5e66afa Compare March 22, 2024 20:41
@niqjohnson niqjohnson force-pushed the tccp/card-details-breadcrumbs branch from 5e66afa to aa3c34a Compare March 25, 2024 14:40
@niqjohnson niqjohnson force-pushed the tccp/card-details-breadcrumbs branch from aa3c34a to 7cdccef Compare March 25, 2024 14:53
Copy link
Member

@contolini contolini left a comment

Choose a reason for hiding this comment

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

Nice work! Once we add dummy data we can write tests for the breadcrumb.

@niqjohnson
Copy link
Member Author

@contolini, you beat me to it—I was going to add some simple breadcrumb tests and xit them out in this PR so they're ready when we get the dummy data in. Just have to be not in a meeting to actually write the tests, which will happen soon.

@niqjohnson niqjohnson added this pull request to the merge queue Mar 26, 2024
Merged via the queue into main with commit 0f3e235 Mar 26, 2024
18 checks passed
@niqjohnson niqjohnson deleted the tccp/card-details-breadcrumbs branch March 26, 2024 04:00
@niqjohnson niqjohnson removed the hold label Mar 26, 2024
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.

3 participants