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

Fix caption templates #3153

Merged
merged 2 commits into from
Oct 15, 2024
Merged

Fix caption templates #3153

merged 2 commits into from
Oct 15, 2024

Conversation

camertron
Copy link
Contributor

What are you trying to accomplish?

A recent PR broke caption templates for certain types of form inputs, causing them not to render at all. The issue was caused by always using the input's configured value: to determine the caption template path, where previously only the name: was used. In cases where the value: was provided for eg. a text input, the code would look for the wrong caption template and, finding no template, would render no caption.

Context

There are two input types for which disambiguating caption template paths by using the value: is necessary: radio button groups and check box groups (but only check box groups submitted as an array). This is because, in each of these cases, the same name is used for each check box or radio button element. In order to know which caption template to render, we also take the value: into consideration when determining the caption template path. For example, a radio button group named "places" that has a child radio button with a value of "seattle" will look for a caption template named places_seattle_caption.html.erb.

Integration

No changes necessary in production.

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

Rather than use the value: to determine the caption template unconditionally, I added the values_disambiguate_template_names? method to all Input classes. This method is called by caption_template_name to determine if the caption template path should be constructed using the value: argument. Doing so should preserve the behavior introduced in #3141 that allows submit buttons to be configured with a value while also only allowing value-based template caption paths to be used for the appropriate input types.

Anything you want to highlight for special attention from reviewers?

Accessibility

  • No new axe scan violation - This change does not introduce any new axe scan violations.

Copy link

changeset-bot bot commented Oct 15, 2024

🦋 Changeset detected

Latest commit: b73bc97

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@camertron camertron merged commit cc1ce7a into main Oct 15, 2024
34 checks passed
@camertron camertron deleted the fix_caption_templates branch October 15, 2024 21:41
@primer primer bot mentioned this pull request Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants