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 Update EditableFormHeading.php to output unique ID attributes to comply with accessibility standards #1312

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

holyavocad0
Copy link
Contributor

@holyavocad0 holyavocad0 commented Aug 5, 2024

Fixes:

Multiple HeaderFields in UserForm creates WCAG Duplicate ID issue #1290

#1290

Description

As a CMS author I want to be able add multiple EditableFormHeading fields to my user and I want all EditableFormHeading ID attributes to be unique so that my UserForm contains valid html.

Manual testing steps

  • Create a CMS UserForm.
  • Add multiple heading fields to the form.
  • Check that all frontend rendered Heading fields have unique ID attributes.
    https://validator.w3.org/
  • check that there is also no impact on the defult bundled JS solutions (that hide/show fields, etc.)

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@GuySartorelli
Copy link
Member

GuySartorelli commented Aug 5, 2024

You've ticked some items in the checklist that are clearly false. CI is not green (it hasn't run - I'm running it now), and your commit message doesn't comply with the guidelines (should start with the prefix FIX).

I've unticked all of the checkbox items except the one about the branch - the branch is definitely correct.. Please go through each checklist item again, read the relevant information, and check if you need to make any changes.
After making any changes that you need to make, please re-tick the checkboxes.

@GuySartorelli GuySartorelli self-assigned this Aug 5, 2024
@NightJar
Copy link
Contributor

NightJar commented Aug 5, 2024

image

  • The target branch is correct
  • Commit contents are all relevant to the issue
  • Commit message prefix appears to be the issue. It is otherwise clear and describes why the change has happened.
  • PR follows guideline
  • Tests are provided; the author has deemed automated testing to be unnecessary.
  • No documentation needs updating
  • CI is green (tests can and should be run locally before the PR is made too - the outcome might already be known)

It is fine (best even) to offer the author the opportunity to adjust the commit message. But the documentation referred to in the template even clearly states that it's OK to not have a prefix.

If you can't find the correct prefix for your commit, it is alright to leave it untagged. The commit will then fall into "Other" category.

This is framed as an accessibility issue, but there is no a11y prefix. Perhaps the author opted for the "Other" category because of that.

If you believe that automated tests are required for this change, then please be clear and state that.

I feel unticking all the boxes is a little heavy handed.

@holyavocad0 holyavocad0 changed the title Update EditableFormHeading.php to output unique ID attributes FIX Update EditableFormHeading.php to output unique ID attributes to comply with accessibility standards Aug 8, 2024
@holyavocad0
Copy link
Contributor Author

@NightJar @GuySartorelli updated. Thanks for the feedback.

@GuySartorelli
Copy link
Member

Thanks for that. Looks like you've added a prefix to the pull request title, but not to the actual commit:
image
Can you please update the actual commit message to include the prefix?

…comply with accessibility standards

Fixes:

Multiple HeaderFields in UserForm creates WCAG Duplicate ID issue silverstripe#1290

silverstripe#1290
@GuySartorelli
Copy link
Member

PHP linting failure in CI is unrelated to this PR

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for submitting this.

@GuySartorelli GuySartorelli merged commit f5cfc22 into silverstripe:6.2 Aug 13, 2024
11 of 12 checks passed
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.

3 participants