Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.48k stars 2.84k forks source link

[HOLD #2934] Enhancements for "Debounce account search typing & fix how validation displays in UI" #2936

Closed mallenexpensify closed 3 years ago

mallenexpensify commented 3 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


On hold for #2934 since that one adds a necessary fix to phone number validation, and this one builds on that

These are enhancements are related to https://github.com/Expensify/Expensify.cash/issues/2934. Please review that job for more details Whomever is assigned that issue can work on these concurrently or after 2934 is fixed. If the assigner of 2934 doesn't want to work on this issue then it's held for anyone else until a week after 2934 is closed (to ensure there aren't regressions)

Enhancements:

When entering an email address or phone number in OptionsSelector's TextInputWithFocuStyles (on all screens where this exists), the app should do a few things:

  1. Show a spinner when user is typing (debounce input, so stop showing spinner 300ms after user stops typing and do validation checks below)
  2. When user has stopped typing for 300ms, do basic front-end validation checks
    • Email validation looks decent for now (Str.isValidEmai and !Str.isDomainEmail)
    • Phone number validation should get more strict, it's super basic at the moment (see Str.isValidPhone)
  3. If above validation passes, do API-level validation for phone numbers only
    • Call the new Expensify API command IsValidPhoneNumber, which validates phone numbers in this format: '(\+?\d{10,15}|\d\d\d-\d\d\d-\d\d\d\d)'.

Make sure validation error messages still show correctly

View all open jobs on Upwork

MelvinBot commented 3 years ago

Triggered auto assignment to @mateocole (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

MelvinBot commented 3 years ago

Triggered auto assignment to @thienlnam (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

Beamanator commented 3 years ago

Removing @thienlnam b/c I'm happy to review proposals πŸ‘

Beamanator commented 3 years ago

Oops, shouldn't have added Exported yet since this job isn't on Upwork yet :O

pranshuchittora commented 3 years ago

Proposal πŸ“„

When entering an email address or phone number in OptionsSelector's TextInputWithFocuStyles (on all screens where this exists), the app should do a few things:

Research/Investigation πŸ•΅πŸ»β€β™‚οΈ

Best Practices πŸ’ƒπŸΌ

Testing Strategy πŸ§ͺ

Expected Delivery Time πŸ“¦

Approx 3-7 days. Considering any possible blockers

Open Questions ❓

Previous Experience πŸ™…πŸΌβ€β™‚οΈ

Beamanator commented 3 years ago

Thanks for the proposal, @pranshuchittora !

Here's some responses to your questions:

pranshuchittora commented 3 years ago

Is that works @Beamanator

https://user-images.githubusercontent.com/32242596/118526334-c0d5a300-b75d-11eb-8f8a-f78ab2c8de60.mp4

Debounce

https://user-images.githubusercontent.com/32242596/118526327-bf0bdf80-b75d-11eb-9434-3d660618fa94.mp4

Beamanator commented 3 years ago

Hey @pranshuchittora !

I like the debouncing, but I believe we should show the spinner while we're waiting for the user to stop typing (for 300ms) and while waiting or the IsValidPhoneNumber response to come back. Also are you planning to make sure the "Concierge" & "pranshu_...@srmuniv..." chats are hidden while the text input is debounced?

pranshuchittora commented 3 years ago

Hi @Beamanator

Also are you planning to make sure the "Concierge" & "pranshu_...@srmuniv..." chats are hidden while the text input is denounced?

Not sure about this but I have decided to keep them because so that user have something to go for. Thinking in terms of the User Experience, it is better to show something instead of nothing. Would love to hear your thoughts on this as well :)

pranshuchittora commented 3 years ago

βœ”οΈ Spinner βœ”οΈ Debounce βœ”οΈ Ignore phone number validation on email βœ”οΈ Edge cases

https://user-images.githubusercontent.com/32242596/118657125-d18f2300-b808-11eb-84d1-b4f614161168.mp4

cc @Beamanator

Beamanator commented 3 years ago

I absolutely don't consider myself an expert in design, which is why I'd recommend we consult with the @Expensify/design group :D

Hey designers! How do y'all feel about the above design, specifically:

  1. When typing in that text box, should we show only the spinner? Or should we also show matching chats?
  2. When the user stops typing and there's some validation error (phone or email) and then keeps typing, the error should go away right?
michelle-thompson commented 3 years ago

When typing in that text box, should we show only the spinner? Or should we also show matching chats?

I think we should show matching chats. I know there have been times where I partially know a phone/name/email and search results fill in the rest, so I think it's helpful.

When the user stops typing and there's some validation error (phone or email) and then keeps typing, the error should go away right?

This makes sense to me, yeah.

shawnborton commented 3 years ago

I think the constant jumping around feels really strange. Could we do something where:

Basically this flow should feel ultra smooth and not bouncey/jumpy like it does in the video, so hopefully the ideas above will help us solve that.

Beamanator commented 3 years ago

Ooh I like these ideas! What do you think @pranshuchittora ?

pranshuchittora commented 3 years ago

Ooh I like these ideas! What do you think @pranshuchittora ?

I agree with all the points. @Beamanator can put a comment consolidating all the new design requirements so that it’s properly documented.

Beamanator commented 3 years ago

Here's the updated design requirements:

Main important thing to keep in mind:

Basically this flow should feel ultra smooth and not bouncey/jumpy

mallenexpensify commented 3 years ago

Created the job in Upwork, made it private and invited @pranshuchittora , once you accept we'll assign this issue to you https://www.upwork.com/jobs/~014262863179787232

mallenexpensify commented 3 years ago

Hired @pranshuchittora in Upwork, assigning this issue now, @Beamanator will be reviewing the PR

pranshuchittora commented 3 years ago

Thoughts @Beamanator @shawnborton @michaelhaxhiu @michelle-thompson

https://user-images.githubusercontent.com/32242596/118997482-51062900-b9a6-11eb-81cd-4142fe2d5e99.mp4

As per this πŸ‘‡πŸΌ

Here's the updated design requirements:

  • While typing:

    • Always show list of matching chats (as you have in your video)
    • Show spinner (either in the text input floating off to the right, or on top of search results (probably centered vertically & horizontally) with search results slightly grayed out but still readable)
    • Clear error message (if one existed)
  • When done typing, error message appears below the search box - there should always be space here, even when error doesn't exist - this should prevent components from jumping around

Main important thing to keep in mind:

Basically this flow should feel ultra smooth and not bouncey/jumpy

Beamanator commented 3 years ago

Looking great!!

One quick comment: When an invalid email address is shown, can you show the message "Please enter a valid email address" below the text field?

Screen Shot 2021-05-20 at 6 58 33 PM
megankelso commented 3 years ago

following Alex's comment, should it perhaps be Please enter a valid email address or phone number in that case?

shawnborton commented 3 years ago

Couple of comments:

pranshuchittora commented 3 years ago

Done @shawnborton @Beamanator

https://user-images.githubusercontent.com/32242596/119545645-ac725580-bdb0-11eb-9cdd-5b2947fbf677.mp4

shawnborton commented 3 years ago

Thanks for putting this together! Candidly, this still does not feel like a great user experience. For example, the error message returns so quickly that you don't even get a chance to fully type out your query: image

Perhaps we can make the time longer from when the user last presses a key versus when the error message shows up?

I also think I might walk back on my idea of having a fixed gap between search box and the results, as I feel like this gap feels a bit visually strange: image

Curious what others think as well.

Beamanator commented 3 years ago

Personally I'm ok with the error message showing up as quick as it does (if you type really quickly you shouldn't get the message until you pause, which makes sense to me).

I do agree that the blank white space under the text box is not very visually appealing... Could we make the error text display above the text box, below 'Search'? Maybe the error message can be shortened so it's 1 line in that case?

pranshuchittora commented 3 years ago

ake the error text display above the text box, below 'Search'? Maybe the error message can be shortened so it's 1 line in that case?

@Beamanator let me know your final thoughts :)

shawnborton commented 3 years ago

Personally I'm ok with the error message showing up as quick as it does (if you type really quickly you shouldn't get the message until you pause, which makes sense to me).

From what I can tell from the recording, you need to type very quickly though. Is there a happy balance we can strike here?

Could we make the error text display above the text box, below 'Search'?

Personally not a fan of this idea as it goes against the pattern we've established elsewhere, where form validation happens as a small message below the input.

Maybe the error message can be shortened so it's 1 line in that case?

That would be awesome, though not sure if we have enough room. In the very least, @pranshuchittora can you make the error text display at a fontSize of 11 and see if that helps?

pranshuchittora commented 3 years ago

@shawnborton in the above-attached video the font size has been changed to 11 as per the feedback received prior to that :)

michaelhaxhiu commented 3 years ago

From what I can tell from the recording, you need to type very quickly though. Is there a happy balance we can strike here?

Yeah I agree with @shawnborton on the quick timing we are displaying the error. It shows up really quickly. Maybe we can add a 1.5 second delay after a user presses the last key?

Also, is there a confirmation button a user clicks after entering a valid email or phone number? Like "Submit" or something? Or is it just enter on the keyboard?

As an idea... I'm wondering if we ought to use something that resembles this grey/green arrow? It could serve a replacement for the loading spinner and a way for a user to confirm their input by tap / click when putting in an email or phone number.

5431

shawnborton commented 3 years ago

I think a submit button would make the experience poor as well, especially given that the chat search is used to quickly jump between chats.

michaelhaxhiu commented 3 years ago

Got it. I looked at other fields in e.cash that resemble this one, and see that you can click the row or hit enter on the keyboard. Let's keep consistent with that theme and ignore my idea.

shawnborton commented 3 years ago

Okay so my latest idea, and not far from where we current are, is that maybe we should just launch the Search pane with no fixed space underneath the search box. I would hope in most cases when people quickly navigate to a different chat, their search query would return results and won't need an error message. When there is indeed an error, let's just show the error message under the box. Bonus points if we could do some kind of animated slideToggle for the height of the error message box so that it feels a little less jarring.

@pranshuchittora @Beamanator @michaelhaxhiu thoughts on trying something like this?

Beamanator commented 3 years ago

I have a similar idea that may help us solve some problems:

What if we only show the error message when there aren't any matching results showing? I think it looks a little weird showing results & an error message at the same time because the user sees "something's wrong with what I'm typing, but there's also some results showing". Example:

Screen Shot 2021-05-27 at 4 41 38 PM

So with this idea of "not showing an error message if the current text matches at least one already-created chat / report", we would basically be doing:

  1. Only show matching results if there's matching results from old chats
  2. New Chat if no chats match & if the typed text is valid
  3. Error message if no chats match & if the typed text is invalid

How about this @shawnborton @pranshuchittora @michaelhaxhiu ? :D

shawnborton commented 3 years ago

I like it! @pranshuchittora thanks for your patience with us as we figure this one out :)

pranshuchittora commented 3 years ago

Sure

Beamanator commented 3 years ago

I believe we're all agreeing we should push forward with this plan right?

michaelhaxhiu commented 3 years ago

Yep!

pranshuchittora commented 3 years ago

I have a similar idea that may help us solve some problems:

What if we only show the error message when there aren't any matching results showing? I think it looks a little weird showing results & an error message at the same time because the user sees "something's wrong with what I'm typing, but there's also some results showing". Example:

Screen Shot 2021-05-27 at 4 41 38 PM

So with this idea of "not showing an error message if the current text matches at least one already-created chat / report", we would basically be doing:

  1. Only show matching results if there's matching results from old chats
  2. New Chat if no chats match & if the typed text is valid
  3. Error message if no chats match & if the typed text is invalid

How about this @shawnborton @pranshuchittora @michaelhaxhiu ? :D

Done πŸš€

https://user-images.githubusercontent.com/32242596/120934227-1bd63680-c71b-11eb-93a2-5f0b0748ea54.mp4

Thoughts ?? P.S> PR has been updated

Beamanator commented 3 years ago

Ooh that looks great! Let's see what the @Expensify/design team things too :D

michelle-thompson commented 3 years ago

Looks good! My only comment is that the search results feel too close to the input. It should look like this, which has 12px padding between input and results: image

pranshuchittora commented 3 years ago

Done @michelle-thompson I have updated the PR

Screenshot from 2021-06-09 23-22-48

michelle-thompson commented 3 years ago

The spacing looks great!

One more comment -- it looks like the focus border is on top of the original input border, you can see a light grey border on all four corners. Can we ensure that the corner radii for the focus matches so we don't see two borders? image

pranshuchittora commented 3 years ago

The spacing looks great!

One more comment -- it looks like the focus border is on top of the original input border, you can see a light grey border on all four corners. Can we ensure that the corner radii for the focus matches so we don't see two borders? image

The focus border is browser behaviour. It depends on the browser and platform (OS). Overriding it might not be feasible. cc @Beamanator

Beamanator commented 3 years ago

@pranshuchittora I think you're right about that - also, there's another issue (https://github.com/Expensify/Expensify.cash/issues/3108) to handle form styling, so as long as this PR doesn't introduce any new form styling issue (which I don't think it does), you can definitely leave this as is :D

mallenexpensify commented 3 years ago

@Beamanator @pranshuchittora , can I get an update on this? Want to post in the parent issue where we're at. Thanks

Beamanator commented 3 years ago

Waiting on @pranshuchittora to respond to my comments or work on requested changes

Beamanator commented 3 years ago

Unassigning for lack of updates - I'm going to clean up requirements & put this on hold for https://github.com/Expensify/App/issues/2934

Beamanator commented 3 years ago

Issue we're holding for hasn't been completely fixed yet

Beamanator commented 3 years ago

Back to weekly since I'm focusing on some other tasks for now

Beamanator commented 3 years ago

Closing this issue since it would be nice to have a fresh start when we're ready