Skip to content

Conversation

@ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Nov 5, 2024

What are the relevant tickets?

Description (What does it do?)

See comments, but roughly:

  • Alters how cards' forwardClicksToLink target is found/marked
  • Fixes card links on /units page
  • Makes whole card clickable on /dashboard/my-lists

How can this be tested?

  1. Cards on /units page should be clickable (whole card)
  2. Whole card (not just title) on /dashbaord/my-lists should be clickable.
    • Check re-ordering these, too.
  3. Other cards should work as before: whole card clickable. (Carousels, search results, Stories + Events)

Comment on lines -199 to +205
* href link as well.
* child anchor with data-card-link="true".
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, if forwardClicksToLink was true, clicking the whole card would click the child anchor with the card's href.

This turned out to be a bit fragile with NextJS. For example, on the /units listing page, the backend currently returns channel_url values like http://localhost:8062/c/unit/mitx/. But NextJS standardizes the href to NOT have a trailing slashing: http://localhost:8062/c/unit/mitx.

We could try to trim the trailing slash, but that seems complicated for things like /some/path/?foo=bar vs /some/path?foo=bar. So instead, let's just find the relevant anchor by explicitly marking it with data-card-link.

Consequently, the Card no longer needs the href, just the Card.Title.

Copy link
Contributor

@gumaerc gumaerc Nov 5, 2024

Choose a reason for hiding this comment

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

Something I'm noticing is that previously, the unit cards were wrapped in an a tag when this was rendered in the browser. Now, following the link seems to be handled by Javascript. Is this a problem for screen readers? It doesn't make logical sense to me either that an href property would be set on the title, as the whole card is supposed to be clickable?

Comment on lines 108 to -145
await waitFor(() => {
const academicSection = screen.getByTestId("UnitSection-academic")
const professionalSection = screen.getByTestId("UnitSection-professional")
units.forEach(async (unit) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is fixing a bug where /units page links were broken. This test should have caught that, except it wasn't actually doing anything because forEach can't be awaited (it always returns undefined) So the test effectively ended on line 144.

@ChristopherChudzicki ChristopherChudzicki changed the title Cc/cards fix Card bug fixes Nov 5, 2024
@ChristopherChudzicki ChristopherChudzicki marked this pull request as ready for review November 5, 2024 16:49
@gumaerc gumaerc self-assigned this Nov 5, 2024
@ChristopherChudzicki ChristopherChudzicki added the Needs Review An open Pull Request that is ready for review label Nov 5, 2024
Copy link
Contributor

@gumaerc gumaerc left a comment

Choose a reason for hiding this comment

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

I'm okay with approving this based on our discussion of why the link forwarding functionality needs to work the way it does. For historical context we discussed how wrapping entire cards in an a tag causes all of the text inside of that card to become the link's textContent property, which makes screen readers read everything inside the card. In order to prevent this from happening, the actual anchor tag is being placed on the title, or on a child element you manually specify using the data-card-link property and then if you set forwardClicksToLink on the parent Card element, clicks on the whole card will go to the nested a tag. We discussed how I think this could be potentially somewhat confusing for new developers, but ultimately decided it was the best solution. We did discuss doing whatever is possible to indicate to developers how this works though, including docstings for Intellisense.

@ChristopherChudzicki ChristopherChudzicki merged commit 65e373e into main Nov 5, 2024
11 checks passed
This was referenced Nov 5, 2024
@rhysyngsun rhysyngsun deleted the cc/cards-fix branch February 7, 2025 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review An open Pull Request that is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants