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

updates template/css so only the title becomes a link #662

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

markconroy
Copy link
Member

@markconroy markconroy commented Nov 27, 2024

Relates to #661

What does this change?

Changes the template for the primary banner paragraph type so that if there is a link for the banner, only the title becomes a link rather than all the content.

I'll create a corresponding PR to update the help text for the field so it doesn't say the whole banner will be linked.
Corresponding PR for LocalGov Paragraphs

How to test

  • Create a subsite
  • Add a Primary Banner to it
  • Give that banner a link
  • Give it a title
  • Give it a summary
  • Check that only the banner title is linked rather than all the content in the banner

Have we considered potential risks?

This might make some changes to existing sites, so we need to communicate that in the release notes.


Thanks to Big Blue Door for sponsoring my time to work on this.

@finnlewis
Copy link
Member

I'm testing this now. Reading the original post #661 I'd be keen to add some magic (CSS / JS ?)to make the whole area clickable still.

@finnlewis
Copy link
Member

Could we do something along these lines? https://wpbeaches.com/fill-a-div-with-a-link-tag/

I guess it might be cleanest with some JavaScript to enhance, re-write the tag, but would this then put us back in the original position of the (I assume) screen reader reading out the whole verbose contents of the link?

@finnlewis
Copy link
Member

@stephen-cox thinks he solved the anchor filling the box issue somewhere else recently...

@finnlewis finnlewis requested a review from stephen-cox December 3, 2024 12:20
@markconroy
Copy link
Member Author

We can add some simple CSS to make the whole thing clickable, quite easily.

a {
  position: relative;
}
a::after {
  position: absolute;
  inset: 0;

That should be enough CSS, and only the actual link text (i.e. the title) will be read out by screenreaders.

@msayoung
Copy link
Member

msayoung commented Dec 9, 2024

Why do we want the whole area to be clickable (in this situation)?

I know this sort of thing (whole card clickabe) is wanted sometimes, I suggest we come up with our preferred methodology and then tell people how to apply it when they want it. Perhaps a spin out another issue?

Note: the issue with Mark's suggestion is that it stops the text being selectable.

@stephen-cox
Copy link
Member

I solve this with JS recently using:

const cards = document.querySelectorAll('.card');
Array.prototype.forEach.call(cards, card => {
  let down, up, link = card.querySelector('.card__title a');
  card.style.cursor = 'pointer';
  card.onmousedown = () => down = +new Date();
  card.onmouseup = () => {
    up = +new Date();
    if ((up - down) < 200) {
      link.click();
    }
  }
});

@markconroy
Copy link
Member Author

@stephen-cox That looks like pretty nice JS. Certainly worth exploring that a bit more.

@msayoung the original request here was to not have the whole banner text area wrapped in an <a> tag. Do you want to merge this PR so we have at least accomplished that? Then we can figure out our approach to "whole-card-clickable" later?

@stephen-cox stephen-cox merged commit 9bb7f5d into 1.x Dec 17, 2024
8 checks passed
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