-
Notifications
You must be signed in to change notification settings - Fork 21
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
Append card title to summary list actions #481
Conversation
✅ Deploy Preview for govuk-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
app/components/govuk_component/summary_list_component/action_component.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - thanks for getting this working, I tried but couldn't easily figure out how to pass the card reference from the summary card to the summary list component.
I’m not sure about action_suffix
as a term, but if you can convince me there’s other use-cases for this other than card titled, then it's probably fine.
The way that the card title is hardcoded to appear within ()
parentheses make it hard to predict what else you might use it for, especially as you can already add visually-hidden text?
The one other thing I don't think this PR does yet is to also add the card title as visually-hidden text to any actions within the header of the card?
I think this is tricker though as currently those actions are just slots that you render a govuk_link_to
in, rather than being built from parameters?
app/components/govuk_component/summary_list_component/action_component.rb
Outdated
Show resolved
Hide resolved
7e5c281
to
7e5501f
Compare
As the summary list can either be have its card automatically rendered around it with `govuk_summary_list(card: { title: "A card" })` or be manually rendered inside a card with `govuk_summary_card(...) { govuk_summary_list(...) }` it makes sense to support both approaches when setting the contextual visually hidden text that follows the action text. This change adds support for the latter by allowing action suffixes to be passed in. If both the card and action_suffix are provided the action_suffix will take precedence, giving designers a little more flexibility with the assistive text should they need it. There might be some situations where the card title doesn't make sense, or it should be lowercased when used in the link, etc.
This extra text provides a bit more context for screen reader users about what the action will affect
Having deliberated a bit over naming I think this is best. Originally it was `card_title` but I think it's misleading because it doesn't actually set a card's title, and `card_title: "xyz"` looks quite similar to `card: { title: "xyz" }`, but they do totally different things. This name is an improvement over `action_suffix` because it tells the dev that it's visually hidden and comes after the action. It's inkeeping with other visually hidden params like `visually_hidden_text` too.
7e5501f
to
20f7d7f
Compare
Merging this. Went with |
This change appends the card title to action links when rendered within a summary card. It follows the upstream change introduced in govuk-frontend version 5.
In the simplest case it works like the reference implementation:
Here, the rendered HTML will now include the card title in the visually hidden text on the action:
Additionally the suffix text can be set manually. This allows it to be set without passing in
card: { ... }
when initialising the summary list. It might be useful when rendering the summary list within an outer card. It is set using theaction_suffix
keyword.This will output the same HTML as the above example.
When both the
card
andaction_suffix
arguments are provided theaction_suffix
takes precedence, which offers a little more flexibility as there might be some cases other text would make more sense than the card's title.Fixes #479, #480