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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 9 commits into from
May 7, 2024
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7795945
Feature: add designSettingsImageAlt to formSettings
JoshuaHungDinh Mar 29, 2024
cc03a07
feature: include type for designSettingsImageAlt
JoshuaHungDinh Mar 29, 2024
462bee2
refactor: pass attachment.alt to onChange handler in media-library co…
JoshuaHungDinh Mar 29, 2024
6ef6a43
feature: include desisngSettingsImageAlt in design header controls on…
JoshuaHungDinh Mar 29, 2024
ceaa11d
feature: add designSettingsImageAlt as primary use for alt tag, with …
JoshuaHungDinh Mar 29, 2024
174e4b2
refactor: replace conditional with shorthand ternary
JoshuaHungDinh Mar 29, 2024
a470e86
Merge branch 'develop' into feature/user-generated-header-img-alt-tags
JoshuaHungDinh Mar 29, 2024
c73d199
refactor: use nullish ?? operator
JoshuaHungDinh Mar 29, 2024
11bc62a
Merge branch 'develop' into feature/user-generated-header-img-alt-tags
JoshuaHungDinh May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/DonationForms/Properties/FormSettings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,12 @@ class FormSettings implements Arrayable, Jsonable
*/
public $designSettingsImageUrl;

/**
* @unreleased
* @var string
*/
public $designSettingsImageAlt;

/**
* @since 3.4.0
* @var string
Expand Down Expand Up @@ -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 •
edited
Loading

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!

$self->designSettingsImageStyle = ! empty($array['designSettingsImageStyle']) ? new DesignSettingsImageStyle(
$array['designSettingsImageStyle']
) : DesignSettingsImageStyle::BACKGROUND();
Expand Down
2 changes: 1 addition & 1 deletion src/DonationForms/resources/app/form/Header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export default function Header({form}: {form: DonationForm}) {
form.settings?.designSettingsImageUrl && (
<HeaderImageTemplate
url={form.settings?.designSettingsImageUrl}
alt={form.settings?.formTitle}
alt={form.settings?.designSettingsImageAlt || form.settings?.formTitle}
color={form.settings?.designSettingsImageColor}
opacity={form.settings?.designSettingsImageOpacity}
/>
Expand Down
4 changes: 2 additions & 2 deletions src/FormBuilder/resources/js/form-builder/src/components/settings/MediaLibrary/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import './styles.scss';
type MediaLibrary = {
id: string;
value: string;
onChange: (url: string) => void;
onChange: (url: string, alt: string) => void;
reset: () => void;
label: string;
help?: string;
Expand Down Expand Up @@ -44,7 +44,7 @@ export default function MediaLibrary({id, value, onChange, label, help, actionLa
// Get media attachment details from the frame state
var attachment = frame.state().get('selection').first().toJSON();

onChange(attachment.url);
onChange(attachment.url, attachment.alt);
});

// Finally, open the modal on click
Expand Down
4 changes: 3 additions & 1 deletion ...rmBuilder/resources/js/form-builder/src/settings/design/general-controls/header/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,16 +123,18 @@ export default function Header({dispatch, publishSettings}) {
label={__('Image', 'give')}
actionLabel={__('Upload Image', 'give')}
value={designSettingsImageUrl}
onChange={(designSettingsImageUrl) => {
onChange={(designSettingsImageUrl, designSettingsImageAlt) => {
dispatch(
setFormSettings({
designSettingsImageUrl,
designSettingsImageAlt,
designSettingsImageStyle,
})
);

publishSettings({
designSettingsImageUrl,
designSettingsImageAlt,
designSettingsImageStyle,
});
}}
Expand Down
1 change: 1 addition & 0 deletions src/FormBuilder/resources/js/form-builder/src/types/formSettings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export type FormSettings = {
pdfSettings: object;
designSettingsImageUrl: string;
designSettingsImageStyle: string;
designSettingsImageAlt: string;
designSettingsLogoUrl: string;
designSettingsLogoPosition: string;
designSettingsSectionStyle: string;
Expand Down
Loading