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

[BREAKING] Align behavior with Primer guidance by default #81

Merged
merged 1 commit into from
Mar 24, 2025

Conversation

joelhawksley
Copy link
Contributor

@joelhawksley joelhawksley commented Mar 17, 2025

Problem

Us folks working on improving the GitHub.com signup flow are looking to address a UX/a11y concern with the auto-check element: it validates on every keystroke.

  • This UX is frustrating to users who may not have provided a complete input yet, such as a partial email address in our case.
  • This UX is frustrating to screen reader users who are spammed with announcements on every keystroke.

Proposed solution

We should modify the behavior to align with https://primer.style/ui-patterns/forms/overview#inline-validation, which states:

Don't attempt to validate an input before the user is done with it. Validation may be performed as the user is typing or making their selection, but only after the first time the input has been validated and the input is in an invalid state. This gives the user early positive feedback by removing the error if the user makes a change that fixes it.

This PR follows up on #76, introducing the new behavior as a breaking change, having validated it across dozens of use cases on GitHub.com.

@Copilot Copilot bot review requested due to automatic review settings March 17, 2025 20:30
@joelhawksley joelhawksley requested a review from a team as a code owner March 17, 2025 20:30
@joelhawksley joelhawksley changed the title [Align behavior with Primer guidance by [BREAKING] Align behavior with Primer guidance by default Mar 17, 2025

Choose a reason for hiding this comment

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

Pull Request Overview

This PR aligns the auto-check element’s validation behavior with Primer guidance by shifting validation from every keystroke to occur on blur.

  • In test/auto-check.js, tests are updated to trigger validation via blur events instead of keypress/input events.
  • In src/auto-check-element.ts, the onlyValidateOnBlur getter is removed and the validation flow in handleChange and check functions is adjusted accordingly.

Reviewed Changes

Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.

File Description
test/auto-check.js Updated tests to simulate blur-triggered validation by removing the opt-in toggle and adding triggerBlur calls.
src/auto-check-element.ts Removed the onlyValidateOnBlur getter and refactored conditional logic to rely on validateOnKeystroke and blur events.
Files not reviewed (2)
  • custom-elements.json: Language not supported
  • examples/index.html: Language not supported
Comments suppressed due to low confidence (2)

test/auto-check.js:32

  • The removal of the only-validate-on-blur attribute in the test markup means that behavior when the opt-in toggle is active is not covered. Add tests to cover both the default behavior and the case when the opt-in toggle is enabled.
<auto-check csrf="foo" src="/success">

src/auto-check-element.ts:209

  • The onlyValidateOnBlur getter has been removed from the auto-check element but related inline comments and logic still reference the opt-in toggle behavior. Update documentation and comments to reflect this removal.
get onlyValidateOnBlur(): boolean {

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

Copy link
Member

@jonrohan jonrohan left a comment

Choose a reason for hiding this comment

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

Couple of questions:

  1. What cases do you think this might fall down in dotcom? Where might this break the ui?
  2. Will you take on migrating those instances?

@joelhawksley
Copy link
Contributor Author

@jonrohan I've already rolled out and tested the new behavior to every use-case in dotcom! I'm confident in moving forward with this change.

@joelhawksley joelhawksley merged commit bbe05d9 into main Mar 24, 2025
4 checks passed
@joelhawksley joelhawksley deleted the validate-on-blur-breaking branch March 24, 2025 14:36
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.

2 participants