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

Make button styles consistent in Expensify.cash #3055

Closed
michaelhaxhiu opened this issue May 21, 2021 · 16 comments · Fixed by #3193
Closed

Make button styles consistent in Expensify.cash #3055

michaelhaxhiu opened this issue May 21, 2021 · 16 comments · Fixed by #3193
Assignees
Labels
Reviewing Has a PR in review

Comments

@michaelhaxhiu
Copy link
Contributor

michaelhaxhiu commented May 21, 2021

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


Expected Result:

Button style attributes should remain consistent across all Expensify.cash platforms (including iOS and Android). More specifically, buttons should behave like so:

  • They are sticky to the bottom of the page/view
  • They won't enable unless the user has completed an action (e.g., typed in a text field, selected a dropdown value, etc.)
    • When the button is disabled it should have a reduced opacity and we should eliminate mouse/touch events for the disabled states.
  • They have a white margin to the sides and below the button to prevent the look of overlapping content (see image1 below)

Actual Result:

Button styles are inconsistent right now. Here's a few examples:

image1:
image

image2:
image

Buttons to modify:

  1. Settings > Profile Save button
  2. Settings > Profile > Add Phone Number/Add Email > Send Validation button
  3. Settings > Change Password Save button
  4. Settings > Payments Add PayPal Account button
  5. + FAB > New Group > Select 1 or more people > Create Group button
  6. + FAB > Request Money > Enter Amount and Press Next > Select an attendee > Enter a comment > Request $XX button.
  7. + FAB > Split Bill > Enter Amount and Press Next > Select 1 or more people > Next button
  8. + FAB > Request Money/Split Bills > Press the currency symbol > Search for a currency using the search bar to focus the keyboard > Select a currency > Confirm button.

Platform:

Where is this issue occurring?

Web
iOS ✔️
Android ✔️
Desktop App
Mobile Web

Version Number: v1.0.50-2
Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/164496

Upwork job link: https://www.upwork.com/jobs/~01b1a9d276904a90c7

View all open jobs on Upwork

@michaelhaxhiu michaelhaxhiu added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label May 21, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label May 21, 2021
@MelvinBot
Copy link

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

@pranshuchittora
Copy link
Contributor

Proposal 📄

Make button styles consistent in Expensify.cash

Investigation 🕵🏻‍♂️

  • Issue is due to improper styling

Approach 👨🏼‍💻

File of concern : Profile, Home...

  1. One of the easiest ways is to remove margin and replace it with padding
  2. Refactor the display properties so that items attached to the bottom when the keyboard is opened.

Discussion with the team might be required to pick the best possible approach

Best Practices 💃🏼

  • No inline styling
  • Flexibility (changes shouldn't break web)
  • Follow React Native best practices

Testing Strategy 🧪

  • Basic render tests

Expected Delivery Time 📦

Approx 1-3 days.

Previous Experience 🙅🏼‍♂️

@Jag96
Copy link
Contributor

Jag96 commented May 24, 2021

@pranshuchittora it looks like you have a few other issues currently assigned to you, so going to give others a chance to write up proposals for this.

For the proposals here, please ensure specific details about what style changes are necessary are included.

@parasharrajat
Copy link
Member

parasharrajat commented May 26, 2021

Here is how I would like to tackle this.

Proposal

  1. To make the button sticky we can use a common technique where we keep the parent flex:1and then add the main content inside a ScrollView and add the fixed button as next sibling element. This is already implemented in the few of the components mentioned above.
  2. For IOS I will wrap all the components in the KeyboardAvoidingView to make the button fixed to bottom.

Skeleton would be something like:

            <ScreenWrapper>
				<KeyboardAvoidingView>
                <HeaderWithCloseButton
                />
                <ScrollView style={styles.flex1}>
                </ScrollView>
                <View style={[styles.ph5, styles.pb5]}>
                    <Button
                        success
                        isDisabled={isButtonDisabled}
                        style={[styles.w100]}
                    />
                </View>
                </KeyboardAvoidingView>
            </ScreenWrapper>
  1. We use our custom Button component for manage the disable/enable state & look.
  2. We will use [styles.ph5, styles.pb5] to maintain the margin around the button.
  3. Tie up the Button's isDisabled prop with form state so that unless form is valid we keep the button deactivated.
  4. Apply the above steps to all the listed component.

@Jag96
Copy link
Contributor

Jag96 commented May 26, 2021

@parasharrajat your proposal looks good, just one question: it looks like the skeleton you're proposing is currently implemented on the ProfilePage, but it is one of the buttons on the list that needs to be changed, what changes will you make there?

When testing on the latest version (1.0.55.0) on the iOS Simulator, it doesn't look like the Save button on the Profile Page is shown on top of the keyboard (see image below). @shawnborton can you confirm that we want the Save button to show on top of the keyboard in this case?

@shawnborton
Copy link
Contributor

Confirming, I think the Save button should be visible above the keyboard.

@parasharrajat
Copy link
Member

Oh, last time I checked it was above the Keyboard so let me just have a look on the latest code.

@parasharrajat
Copy link
Member

parasharrajat commented May 26, 2021

@Jag96
Oh sorry I though we are already using the https://github.com/Expensify/Expensify.cash/blob/c11594b845351a638f4782e7e2d6dc0a6772294d/src/libs/KeyboardAvoidingView/index.ios.js. we don't need it for Android.

But it seems we have only added it one place so far so I will wrap all the components in the KeyboardAvoidingView to make the button fixed to bottom. Updating the proposal...

@Jag96
Copy link
Contributor

Jag96 commented May 26, 2021

@parasharrajat your proposal looks good 👍 Please submit a proposal on Upwork, once it's approved you can start on a PR

@michaelhaxhiu
Copy link
Contributor Author

Hired @parasharrajat on Upwork. Feel free to start on your PR!

@michaelhaxhiu
Copy link
Contributor Author

Will pay on June 18 (7 days after PR merged to allow for regression handling)

@michaelhaxhiu
Copy link
Contributor Author

Payment will be sent tomorrow.

@arielgreen
Copy link
Contributor

@michaelhaxhiu please pay this out ASAP.

@arielgreen arielgreen reopened this Jun 24, 2021
@arielgreen
Copy link
Contributor

Ah, nvm, it's paid. Please be sure you're ending contracts. I will do so.

@michaelhaxhiu
Copy link
Contributor Author

Please be sure you're ending contracts.

Shoot, sorry! I did forget to end the contract.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewing Has a PR in review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants