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

Feature: add custom alt tag to "HeaderImage" #7335

Merged
merged 9 commits into from
May 7, 2024

Conversation

JoshuaHungDinh
Copy link
Contributor

@JoshuaHungDinh JoshuaHungDinh commented Mar 29, 2024

Resolves # GIVE-568

Description

This introduces a new setting that allows users to include custom alt tag information for the HeaderImage setting. This is done by passing attachment.alt to the onChange handler inside the media-library component. If no alt tag information is available the alt tag will use the form title as a fallback.

The media-library component will assign an empty string if the information is not filled out, so a Nullish coalescing operator (??) was added to the formSettings and the Logical OR (||) was used in the HeaderTemplate to ensure the form title properly renders as a fallback.

Affects

HeaderImage
FormSettings

Visuals

alt-tag-test.mov

Testing Instructions

  • Create a v3 form
  • Add a an image for the Header, while selecting an image on the right of the media uploader you should see an input to add alt tag information. Assign custom alt tag text.
  • Right click HeaderImage in design preview to open up the console and view the img tag, you should see the alt property is assigned your custom text. Also verify on the frontend page.
  • Go back to the builder and remove the alt tag information.
  • View the element in the console for both design preview & on the frontend. Verify that the alt tag has been replaced by the form title.

Pre-review Checklist

  • Acceptance criteria satisfied and marked in related issue
  • Relevant @unreleased tags included in DocBlocks
  • Includes unit tests
  • Reviewed by the designer (if follows a design)
  • Self Review of code and UX completed

@JoshuaHungDinh JoshuaHungDinh marked this pull request as ready for review March 29, 2024 16:10
@JoshuaHungDinh JoshuaHungDinh changed the title Feature: add custom alt tags to "HeaderImage" Feature: add custom alt tag to "HeaderImage" Mar 29, 2024
Copy link
Member

@kjohnson kjohnson left a comment

Choose a reason for hiding this comment

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

@JoshuaHungDinh could you add a test to FormSettings for the fallback value?

@@ -314,6 +320,7 @@ public static function fromArray(array $array): self
) ? $array['pdfSettings'] : [];

$self->designSettingsImageUrl = $array['designSettingsImageUrl'] ?? '';
$self->designSettingsImageAlt = ! empty($array['designSettingsImageAlt']) ? $array['designSettingsImageAlt'] : $self->formTitle;
Copy link
Member

Choose a reason for hiding this comment

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

This could be simplified to a shorthand ternary (?:). That is, if the array key exists but is an empty string or null (otherwise you'll get undefined index notice).

Copy link
Member

Choose a reason for hiding this comment

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

Note that the shorthand ternary behaves differently that the null coalescing operator (??).

Copy link
Contributor Author

@JoshuaHungDinh JoshuaHungDinh Mar 29, 2024

Choose a reason for hiding this comment

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

Good to know next time. I seem to have had mixed the FormSettings and the HeaderTemplate conditionals. The Nullish coalescing operator in fact does work for the FormSettings. At the time of testing I did not have the proper conditional in the template & had mixed the two up. c73d199

Unless Im mistaken I couldn't find FormSetting Tests but I would be happy to make them for the fallbacks. Maybe in a new PR?

Copy link
Member

Choose a reason for hiding this comment

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

While I often suggest moving additional work in a separate PR, in the case of a test I think it would be important to add it as a part of the PR. I see that the class does not have a test yet,so if you'd like to jump on a pairing session I'd be happy to work together on adding the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @kjohnson , I wanted to let you know that I saw this. I was thinking we could connect on Friday, if you're available?

Copy link
Member

Choose a reason for hiding this comment

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

@JoshuaHungDinh I dropped the ball here and meant to follow-up on Friday. My apologies. To get you unblocked I'll add my review for this PR and then we can setup a test for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds great. No worries, same here. It's just Fun-Friday work. No real expectation on a timeline for this. Thank you!

Copy link
Member

@rickalday rickalday left a comment

Choose a reason for hiding this comment

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

Passed manual QA tests.

@jonwaldstein jonwaldstein merged commit 63754b4 into develop May 7, 2024
20 checks passed
@jonwaldstein jonwaldstein deleted the feature/user-generated-header-img-alt-tags branch May 7, 2024 17:10
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