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

feat: form layout improvements (resolves #75) #76

Merged
merged 13 commits into from
Aug 23, 2021

Conversation

greatislander
Copy link
Collaborator

@greatislander greatislander commented Aug 20, 2021

This PR makes a number of improvements to Looseleaf's form handling based on suggestions that emerged while working on the platform. As follows:

  • Larger type for <legend>
  • Section dividers between successive <fieldsets>
  • Don't use pseudo content for checkboxes (the checkbox was being announced by screen readers)
  • Set width to auto for <input type="submit"> and <input type="reset">
  • Field hint styling
  • Field error message styling

Testing

Review all variants of the following components:

As well as the combined Form layout.

@greatislander greatislander added the enhancement New feature or request label Aug 23, 2021
@greatislander greatislander self-assigned this Aug 23, 2021
@greatislander greatislander marked this pull request as ready for review August 23, 2021 14:07
@greatislander greatislander linked an issue Aug 23, 2021 that may be closed by this pull request
6 tasks
@jobara
Copy link
Member

jobara commented Aug 23, 2021

I noticed in the referenced WebAIM article the section on aria-invalid. Which suggests that you set it when an error is present. Looking at the ARIA spec, aria-invalid should likely be set to true when an error is present. Also it looks like we should be using aria-errormessage to reference the error message.

@jobara
Copy link
Member

jobara commented Aug 23, 2021

@greatislander you may want to look into the support for aria-errormessage as I haven't used it before. Maybe @lliskovoi might have some thoughts about it.

@greatislander
Copy link
Collaborator Author

I found this, @jobara — sounds like support for aria-errormessage may be not reliable across browser and screenreader combos: https://webaim.org/discussion/mail_thread?thread=8911

@greatislander
Copy link
Collaborator Author

@jobara I've added aria-invalid and documented this change. I'll await some feedback from @lliskovoi on the feasibility of using aria-errormessage instead of/in addition to aria-describedby.

@jobara
Copy link
Member

jobara commented Aug 23, 2021

@greatislander the thread you referenced mentioned two issues: nvaccess/nvda#8318 and FreedomScientific/standards-support#83. They are both still open. Sounds like at least some of the browsers may be exposing things correctly, and that support for JAWS is improving.

@greatislander
Copy link
Collaborator Author

@jobara I think given that support is not consistent across browsers/screen readers it makes sense to stick with using a combo of aria-invalid to indicate that the input is in an error state and aria-describedby to link the input to the message.

@greatislander greatislander merged commit cdfc8be into main Aug 23, 2021
@greatislander greatislander deleted the feat/form-layout-improvements branch August 23, 2021 17:09
Copy link

@cherylhjli cherylhjli left a comment

Choose a reason for hiding this comment

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

looks great! (there's no approve option on here, let me know if I have to do something else)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve form layouts
3 participants