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(admin-panel): trim whitespace from admin panel search bar #12706

Merged
merged 1 commit into from
May 4, 2022

Conversation

millmason
Copy link
Contributor

@millmason millmason commented Apr 29, 2022

Because:

  • When copying and pasting values into the search bar, it's easy to accidentally include whitespace either before or after the copied value. Currently, this will result in the search not returning a result (as it will search for the entire input, including whitespace.)

This pull request:

  • Trims the search input value before we submit it to the search (in the handleSubmit action, in the input length check that determines whether or not to show suggestions, and also in the search that returns suggested matching emails.) There is a slightly simpler way to this -- we could just trim
    "event.target.value" before it's set into state using setSearchInput, but this could result in the user trying to type
    in the input (with spaces, for example) and thinking they couldn't interact with the search input (since the value of the search input would be constantly trimmed). While slightly more complicated, I think this is a safer solution.

  • There also seemed to be a duplicate setSearchInput action happening in onTextChanged, which I have removed. onTextChanged seems to only ever be called by handleChange, which already calls setSearchInput.

Closes #12567

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots

Before (whitespace before or after search input value is included in query term and returns nothing):
Screen Shot 2022-04-29 at 10 27 25 AM

After (whitespace is trimmed and search returns correct record):
Screen Shot 2022-04-29 at 10 25 25 AM

Because:

* When copying and pasting values into the search bar, it's easy to
  include whitespace either before or after the copied value. Currently,
this will result in the search not returning a result.

This commit:

* We trim the search input value before we submit it to the search.
  There is a slightly simpler way to this -- we could just trim
"event.target.value", but this could result in the user trying to type
in the input (with spaces, for example) and thinking they couldn't type.

Closes #
@millmason millmason requested a review from a team as a code owner April 29, 2022 17:28
@dschom dschom self-requested a review May 3, 2022 21:14
Copy link
Contributor

@dschom dschom 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 the fix! I have one very small nitpick, that you should feel free to ignore if you like. It seems like this could have been done in fewer lines on the server side or whenever the value is set (as opposed to when it's read). Either of these approaches might be seen as a bit more DRY and indicate a more maintainable solution.

@millmason
Copy link
Contributor Author

@dschom That's a really interesting idea, I'll look into it!

@millmason
Copy link
Contributor Author

@dschom I looked into this and we can transfer trimming the strings out of the frontend, but it will be handed off to GraphQL, so I think it would be harder to maintain (since there are a few different GraphQL methods, so the duplication would be obscured.) My issue with trimming the string where it was set is that I think it's a confusing interaction for the user -- if the user enters whitespace into the search input, it will look as if the search input is broken (because the whitespace characters don't show up.) That being said, the only reason why the whitespace was introduced was due to copy-pasting values -- not due to a user typing whitespace into the search input. Also maybe we should prioritize code readability over covering all corners of the user experience since this is purely admin-facing. What do you think?

@dschom
Copy link
Contributor

dschom commented May 4, 2022

@millsoper I think it's good to go then! I had my suspicions that doing it in the set resulted in a glitchy ux in some cases. This was truly just a nitpick, and I was mostly was curious if/why this approach was used over the others.

@millmason millmason merged commit df2e73a into main May 4, 2022
@millmason millmason deleted the fxa-4910-trim-search-input branch May 4, 2022 20:31
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.

email input on admin panel can have whitespace on edges
2 participants