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

Remove unnecessary id attribute. #22347

Merged
merged 1 commit into from
Jan 2, 2022

Conversation

braders
Copy link
Contributor

@braders braders commented Dec 31, 2021

Overview

The min_amount ID was used for both a tr and a input element, breaking the accessibility link between the label and input elements.

Before

The "Minimum Amount" label was not semantically linked with the appropriate input element due to a duplicate ID. The duplicate ID has been removed. This is on the settings page for a single price set. (civicrm/admin/price?action=update&sid=123)

After

The duplicate ID attribute been removed.

Technical Details

There are a handful of JS references to the min_amount ID. However, these all apply to other templates, and all assume that #min_amount is an input element (for example, because they read or write the elements value property).

@civibot
Copy link

civibot bot commented Dec 31, 2021

(Standard links)

@civibot civibot bot added the master label Dec 31, 2021
@colemanw
Copy link
Member

Thanks @braders that makes sense.
I think test failures are unrelated. @civicrm-builder retest this please.

@colemanw
Copy link
Member

@braders I think it would be safer to leave financial_type_id_row alone as it's not hurting anything. I agree it's unused in civicrm-core but it may have been added for the sake of an extension - there are a few financial extensions out there that do alterations to forms like this one.

The min_amount ID was used for both a tr and a input element, breaking accessibility links
@braders braders changed the title Remove unnecessary id attributes. Remove unnecessary id attribute. Jan 1, 2022
@braders
Copy link
Contributor Author

braders commented Jan 1, 2022

@braders I think it would be safer to leave financial_type_id_row alone as it's not hurting anything. I agree it's unused in civicrm-core but it may have been added for the sake of an extension - there are a few financial extensions out there that do alterations to forms like this one.

That's fair. I've re-pushed, and updated the PR description to match.

@colemanw
Copy link
Member

colemanw commented Jan 1, 2022

@civicrm-builder retest this please

@colemanw colemanw merged commit 476bfe2 into civicrm:master Jan 2, 2022
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