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.6k stars 2.93k forks source link

Extra margin over maximum limit reached error on login page #20297

Closed kavimuru closed 1 year ago

kavimuru commented 1 year 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!


Action Performed:

  1. Open the app
  2. Open settings
  3. Open profile
  4. Open contact methods
  5. Click on Add
  6. Add new contact method
  7. Reopen and keep on clicking resend magic code until you see 'maximum limit reached' error
  8. Sign out from the app
  9. Login again with same user and observe margin above 'Maximum limit reached' error
  10. Write any phone number without country code to trigger insert country code error and observe margin above that error

    Expected Result:

    App should keep consistent margin above all the errors displayed on login page

    Actual Result:

    App has 20px more margin on top for 'Maximum limit reached' error then any other errors on login page

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.3.24.4 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/43996225/15e2299d-16e9-4f94-8e01-14b8aac982f2

https://github.com/Expensify/App/assets/43996225/c5267e99-eeef-433f-ba64-85e9d7632940

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1685719639488529

View all open jobs on GitHub

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

dukenv0307 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Extra margin over maximum limit reached error on login page

What is the root cause of that problem?

We are setting margin-top: 20px for FormAlertWithSubmitButton component https://github.com/Expensify/App/blob/96adb8e58e53fd76c7a316234dab322643dfd479/src/pages/signin/LoginForm.js#L208 If there is no message, It looks good

Screenshot 2023-06-05 at 16 44 47

But when there is an error message the margin-top will be 28px

Screenshot 2023-06-05 at 16 47 41

because the error message also has margin-top: 8px https://github.com/Expensify/App/blob/96adb8e58e53fd76c7a316234dab322643dfd479/src/components/FormHelpMessage.js#L39

Note that: I also checked other error messages in our app and see that in all other places the margin-top: 8px will be added above the error message

What changes do you think we should make in order to solve the problem?

https://github.com/Expensify/App/blob/96adb8e58e53fd76c7a316234dab322643dfd479/src/pages/signin/LoginForm.js#L208 In here we only should add margin-top when there is no error message by checking like this

<View style={[!serverErrorText && styles.mt5]}>

Result

Screenshot 2023-06-05 at 16 49 40
dhairyasenjaliya commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?


//LoginForm.js 
                    this.props.isVisible && (
-                        <View style={[styles.mt5]}>
                            <FormAlertWithSubmitButton 
-                                containerStyles={[styles.mh0]}
+                            containerStyles={[styles.mh0, !_.isEmpty(serverErrorText) ? {} : styles.mt5]} 
                            />
-                        </View>
                    )
hungvu193 commented 1 year ago

IMO, @dhairyasenjaliya 's proposal hit the 🎯

joekaufmanexpensify commented 1 year ago

I can reproduce this. This is a pretty minor issue, but the padding is different here. @shawnborton curious for your take here. We should standardize on less padding here, right? (So using the padding of the phone number error in the bottom of the two videos in OP)?

shawnborton commented 1 year ago

Hmm I don't know if the margin/padding thing is really that big of a deal, I think we can close this personally.

joekaufmanexpensify commented 1 year ago

Sounds good. Closing!