Skip to content

Conversation

@benjaminleonard
Copy link
Contributor

Draft because I want to get people's thoughts on it. I think it might be easier to discover and a nicer experience to make the entire link cell clickable. Presently just the text is clickable.

CleanShot 2024-01-15 at 17 06 57

@vercel
Copy link

vercel bot commented Jan 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Jan 22, 2024 11:48am

@benjaminleonard
Copy link
Contributor Author

benjaminleonard commented Jan 16, 2024

The Link is positioned absolutely to work around the padding on its element. Wondering if it might however cause issues with screen readers? Perhaps expecting text to be inside the link element.

Couple of other approaches:

  1. Negative margin
<Link
  className="-m-3 -mr-8 flex h-[calc(100%+1.5rem)] w-[calc(100%+2.75rem)] items-center p-3 pr-8 text-sans-semi-md text-default hover:underline"
  aria-label={value}
  to={makeHref(value)}
>
  {value}
</Link>

Potential downside here is keeping the margins of the parent cell and this in sync

  1. Use a has selector to check if there's a link cell and if there is remove the padding and apply it directly

  2. Use a block to push out the boundary of the link element

<Link
  className="flex h-full w-full items-center text-sans-semi-md text-default hover:underline"
  aria-label={value}
  to={makeHref(value)}
>
  {value}

  <div className="absolute inset-0" />
</Link>

@benjaminleonard
Copy link
Contributor Author

benjaminleonard commented Jan 17, 2024

Third option seems to work the best with minimal jank. Need to do some cross-browser testing.

Worth also saying, since using the console with and without this, it does feel like a significant user experience boost.

@david-crespo
Copy link
Collaborator

What if it's just underlined all the time? Then you probably wouldn't need to change the target.

@benjaminleonard
Copy link
Contributor Author

benjaminleonard commented Jan 18, 2024

Worth including as an option, I think it gets a bit visually intense but lets give it a go.

I do think that can be separate, since it's also about the comfort of the size of the clickable area. But that doesn't necessarily need to be the whole cell.

@just-be-dev
Copy link
Contributor

You can definitely tell the improvement on mobile (not that our layout is optimized for that generally). It does feel pretty good. I agree that the third option seems the least fragile, but I think you should definitely add an inline comment saying what its purpose is.

Copy link
Collaborator

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

Looks good to me. I kinda like seeing a hover background color on the cell but it would be redundant with the cursor and link underline.

@benjaminleonard benjaminleonard marked this pull request as ready for review January 22, 2024 11:48
@benjaminleonard benjaminleonard enabled auto-merge (squash) January 22, 2024 11:55
@benjaminleonard benjaminleonard merged commit d80d2e7 into main Jan 22, 2024
@benjaminleonard benjaminleonard deleted the link-cell-hover branch January 22, 2024 11: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.

4 participants