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

Emit an input event primer-text-field#clearContents. #3062

Merged
merged 4 commits into from
Sep 10, 2024

Conversation

dombesz
Copy link
Contributor

@dombesz dombesz commented Sep 5, 2024

What are you trying to accomplish?

I want to make the primer-text-field clear button to trigger an input event in the field. This makes the clear button logic to behave the same way as it was deleted by the user manually.

A good example for this behaviour is to use the primer-text-field as a search field, triggering a background request to refresh the results when its value is changed. Without emitting the input event on the #clearContents, refreshing the results would not be triggered.

A possible workaround would be to bind the background request directly on the clear button too. Unfortunately there is no simple way to bind events to the clear button, because event binding to the clear button is not possible via the primer-text-field component declaration.

Integration

Does not require any code change in production.

List the issues that this change affects.

Closes #3061

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

The solution is trigger an input event when the primer-text-field clear button is clicked.

  • Fixes axe scan violation - This change fixes an existing axe scan violation.
  • No new axe scan violation - This change does not introduce any new axe scan violations.
  • New axe violation - This change introduces a new axe scan violation. Please describe why the violation cannot be resolved below.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

Having the `#clearContents` emit an `input` event, it integrates better
whenever an `input` event listener is bound to the `primer-text-field`
components. A classic example is to use the `primer-text-field` as a
search field, which triggers a background request when the field's value
is changed. Without emiting the `input` event on #clearContents, the
filtering background request would not be triggered.
@dombesz dombesz requested a review from a team as a code owner September 5, 2024 19:46
@dombesz dombesz requested a review from mperrotti September 5, 2024 19:46
Copy link

changeset-bot bot commented Sep 5, 2024

🦋 Changeset detected

Latest commit: b7bb9fe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jonrohan jonrohan requested a review from camertron September 9, 2024 21:30
Copy link
Contributor

@camertron camertron left a comment

Choose a reason for hiding this comment

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

Awesome, thank you! Just a few small things, then this is good to 🚀

test/system/alpha/text_field_test.rb Outdated Show resolved Hide resolved
lib/primer/forms/primer_text_field.ts Outdated Show resolved Hide resolved
Co-authored-by: Cameron Dutro <[email protected]>
@dombesz dombesz requested a review from camertron September 10, 2024 06:43
@camertron
Copy link
Contributor

Ah I forgot to mention this last time @dombesz, but could you also add a changeset? Running npx changeset should get you going.

@dombesz
Copy link
Contributor Author

dombesz commented Sep 10, 2024

Ah I forgot to mention this last time @dombesz, but could you also add a changeset? Running npx changeset should get you going.

@camertron I already have 1 here, do I need to add a new one?

@camertron
Copy link
Contributor

Ah sorry I missed it, carry on!

@camertron camertron merged commit b45ef04 into primer:main Sep 10, 2024
33 of 35 checks passed
@dombesz dombesz deleted the feat-emit-input-on-clear branch September 10, 2024 21:31
@primer primer bot mentioned this pull request Sep 10, 2024
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.

Clearing the primer-text-field's content via the clear button does not trigger an input event.
2 participants