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

[Hold of payment Aug 17th] Update form styles on Expensify.cash mobile app #3108

Closed
Christinadobrzyn opened this issue May 25, 2021 · 54 comments · Fixed by #3414
Closed

[Hold of payment Aug 17th] Update form styles on Expensify.cash mobile app #3108

Christinadobrzyn opened this issue May 25, 2021 · 54 comments · Fixed by #3414
Assignees
Labels
Design External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented May 25, 2021

Platform - version: E.cash app v1.0.38.-1

Action Performed (reproducible steps):

  • Go to Expensify.cash
  • View any field on the mobile app (Phone or Email sign-in filed, preferences, change password, search, etc)
  • Currently, you'll see a grey field label on an empty field > tap the field to select it
  • The field will go white with a blue border > the grey field will disappear
  • We want to change this so the grey field label remains while typing
  • Here's examples of the current layout and the desired layout:

Current:
image

Desired:
image

Specific specs:

  • Text size should be 13pt for field label and 15pt for text

  • Keeping our existing border style (blue when focused/active, red when error/active) and button style (green for primary actions)

  • Field inside the box should be white (both in a selected field and unselected field)

  • Field label text should be grey (both in a selected field and unselected field)

  • These field changes need to be made to all fields on the mobile app

Expected Result:

  • Every selected field without an error should show a grey field label at the top of the field, have a blue border and a green highlighted action button

  • Every selected with an error field should show a grey field label at the top of the field, have a red border and a green highlighted action button

Actual Result:

  • Currently, every selected field hides the grey field label

Notes/Photos/Videos:

Here's a view of how this should display:

image

PLEASE NOTE: This should not be applied in the main text box where "Write something" displays, this should keep the following behaviour:

image

@Christinadobrzyn Christinadobrzyn self-assigned this May 25, 2021
@Christinadobrzyn
Copy link
Contributor Author

Waiting for Eng review of this issue - https://github.com/Expensify/Expensify/issues/157830 - before uploading to Upwork

@kakajann
Copy link
Contributor

Hello, I would like to work on this. One of my previous work was mostly about styling (Growl Notification ).

@Christinadobrzyn
Copy link
Contributor Author

I'm going to be ooo May 31 - June 7th so assigning this to Kadie to post in Upwork once we have a thumbs up on the internal GH - https://github.com/Expensify/Expensify/issues/157830

@MelvinBot
Copy link

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

@kadiealexander
Copy link
Contributor

@Beamanator I'm yet to post this on Upwork but we already have proposals so I've added the label.

@Beamanator
Copy link
Contributor

@kakajann Thanks for your interest in this issue! Would you please provide write up a proposal of the work you'd plan to do? Please also ask any questions you may have about the desired "Expected Result".

@kakajann
Copy link
Contributor

kakajann commented Jun 2, 2021

Proposal

React Native doesn't support :focus selector on styling. So, we have to create custom TextInput with react natives TextInput if we want to change TextInputs border color when user focus on it.

Example:

import React, { Component } from 'react';
import { TextInput, StyleSheet } from 'react-native';

const styles = StyleSheet.create({
  textInput: {
    fontSize: 15,
    borderColor: '#dddddd'
  },
  textInput: {
    borderColor: '#00ff00'
  },
})

class ExpensifyTextInput extends Component {
  constructor() {
    super()

    this.state = {
      focused: false
    }
  }

  render() {
    const {focused} = this.state
    return (
      <TextInput
        style={[styles.textInput, focused && styles.textInputOnFocus]}
        onFocus={() => this.setState({focused: true})}
        onBlur={() => this.setState({focused: false})}
      />
    )
  }
}

export default ExpensifyTextInput

I will also implement specific specs that mentioned in the issue.

Actual Result:
Currently, every selected field hides the grey field label

And this is because outline hasn't disabled in here

@Beamanator
Copy link
Contributor

@kakajann From your proposal I'm not sure you've clearly read / understood what changes are needed.

So, we have to create custom TextInput with react natives TextInput if we want to change TextInputs border color when user focus on it.

The spec says "Keeping our existing border style (blue when focused/active, red when error/active)", so you shouldn't need to build anything extra for border colors. The main change in this issue is about the field placeholder / label text, can you explain how you think you would implement those requested changes?

@kadiealexander
Copy link
Contributor

@mnrafg
Copy link

mnrafg commented Jun 4, 2021

Hello Expensify Team,

I hope you are all doing fine.

About Me

I'm Nasir Rahimi a freelance full-stack developer with almost a decade of practical experience and following is my Upwork profile link.
https://www.upwork.com/freelancers/~0189e2e63b0663c9b4

Proposal

I suggest two options to achieve the expected and resulting TextInput.

First Option: Customize and Use Third-Party Component

It is an option to use such a component from a third-party stable library, which has almost the same functionality but needs some style changes to meet the expected UI requirements an example I can mention here now is the extended TextInput component from the react-native-paper library.

You can see the examples by visiting the following link: https://callstack.github.io/react-native-paper/text-input.html

Second Option: A Custom Extension of TextInput Component

The second option could be the straightforward extension of the built-in ReactNative TextInput, which will result in our own new TextInput component.

Note: the following code is a basic draft idea about the implementation for the complete solution I'll cheat from the above-mentioned react-native-paper library and carefully study how they extended the native TextInput component for that you can read the TextInputOutlined component code by visiting the following link.

https://github.com/callstack/react-native-paper/blob/40253f5b07a87812315c16a61a6bc19edb077638/src/components/TextInput/TextInputOutlined.tsx

class TextInput extends React.Component {
  // ...
  render() {
    const {label, ...inputProps} = this.props;

    return (
      // Wrapper View
      <View>
        {
        // An extended and customized View as Outline
        // which will be visible as input outline or border.
        }
        <Outline />
        {
        // An extended and customized Text
        // which will be visible as input label.
        }
        <Label value={label} />
        {
        // An extended and customized Text
        // which will be visible as input label.
        }
        <TextInput {...inputProps} />
      </View>
    )
  }
}

Solution

I hope you have got the path I'll follow for solving this issue, but I want to mention that I'll need a few days for this to implement because I'm currently having other projects as well.

Regards,
Nasir Rahimi

@kakajann
Copy link
Contributor

kakajann commented Jun 5, 2021

@Beamanator , No inputs currently change its border color on focus. (I'll attach a video below) That's why I suggested to create custom TextInput. Also, creating a custom TextInput component will make it easy to implement shrinking labels.

I can provide a working sample if you me to do. It's easier than explaining :)

And here is current inputs (that doesn't change its border color)

Screen.Recording.2021-06-05.at.7.10.02.PM.mov

@Beamanator
Copy link
Contributor

@mnrafg Thanks for your proposal and your multiple ideas! I believe your second solution (A Custom Extension of TextInput Component) could be the best solution for what we're looking for, but I am planning to follow up with our team internally to make sure nobody has any concerns about this plan :D I'll get back to you soon! (most likely tomorrow)

@Beamanator
Copy link
Contributor

Beamanator commented Jun 7, 2021

@kakajann Thanks for the video, I do see what you mean now. I believe react-native-web is adding data-focusvisible-polyfill="true" to our inputs, which is giving our text fields the blue borders on practically all fields in web, different colored borders in mobile web (for Android at least), and no borders in mobile (except for the chat message text field). Now I'm pretty sure the borders I'm seeing are browser-specific, not necessarily related to something react-native-web is doing.

Your solution could work great for border styling, but I didn't see a clear path forward for how you planned to implement the label-shrinking in the new custom TextInput component. Feel free to add a proposal with details on that if you'd like!

@Beamanator Beamanator added the External Added to denote the issue can be worked on by a contributor label Jun 7, 2021
@MelvinBot
Copy link

Triggered auto assignment to @stephanieelliott (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Jun 7, 2021
@Beamanator Beamanator added Weekly KSv2 and removed Daily KSv2 labels Jun 7, 2021
@kakajann kakajann mentioned this issue Jun 8, 2021
13 tasks
@Beamanator
Copy link
Contributor

@kakajann I added the Design label to assign someone from the Design team to look at the PR - looks like @sylveawong is the lucky one :) Also @kakajann could you remove [WIP] from your PR title if it's ready for review, and assign pullerbear?

@shawnborton
Copy link
Contributor

I don't mind grabbing this one as @kakajann already tagged me in the PR.

@shawnborton shawnborton assigned shawnborton and unassigned sylveawong Jul 5, 2021
@kakajann
Copy link
Contributor

I have no idea how to request a review from pullerbear. any help would be appreciated)

@roryabraham
Copy link
Contributor

@Beamanator @kakajann Readonly permissions (as all our Open-Source contributors have) aren't enough to request a reviewer in GitHub. See https://docs.github.com/en/organizations/managing-access-to-your-organizations-repositories/repository-permission-levels-for-an-organization for more details.

@kakajann We have PullerBear set up as a code owner, so as soon as you create a PR his review will be automatically requested, and then an Expensify engineer will be automatically assigned to review your code. So you shouldn't need to do anything 😁

@Beamanator
Copy link
Contributor

@roryabraham for reference, here is @kakajann 's PR: #3414

It looks like when he originally created the PR, it did assign pullerbear & later Shawn re-requested pullerbear to review, which is how I'm the current reviewer 👍 I was on vacation yesterday so I'll get to this today!

Readonly permissions (as all our Open-Source contributors have) aren't enough to request a reviewer in GitHub. See https://docs.github.com/en/organizations/managing-access-to-your-organizations-repositories/repository-permission-levels-for-an-organization for more details.

That's great to know! Thanks for sharing :)

@Christinadobrzyn
Copy link
Contributor Author

Christinadobrzyn commented Jul 26, 2021

Just checking on this PR, it looks like we're still working through some checks. Let me know if that's incorrect!

@kakajann
Copy link
Contributor

I had to work on this for more than 2 months because of delayed code reviews and unexpected conflicts. Also, I got to add new features.

Therefore, I am requesting to increase the budget for this work.

@parasharrajat
Copy link
Member

I support @kakajann here. PR looks heavy.

@Beamanator Beamanator added the Reviewing Has a PR in review label Jul 30, 2021
@Beamanator
Copy link
Contributor

Beamanator commented Jul 30, 2021

@kakajann thanks for bringing this up, we're discussing internally and I definitely agree you should get a pay bump for this big task. Can you please give us an estimated # of hours you've worked on this related PR?

@kakajann
Copy link
Contributor

I have no idea to be honest. Yesterday only, spent 6 hours 36 minutes (wakatime's result) to refactor

@MelvinBot
Copy link

@shawnborton, @Beamanator, @kakajann, @Christinadobrzyn it looks like no one is assigned to work on this job.
Please double the price or add a comment stating why the job isn't being doubled.

@Beamanator
Copy link
Contributor

Job price in Upwork will be doubled today 👍

@Christinadobrzyn
Copy link
Contributor Author

Christinadobrzyn commented Aug 4, 2021

It looks like the original Upwork job expired so I created a new one with the $500 price and hired @kakajann

Internal job post - https://www.upwork.com/ab/applicants/1422830164452577280/job-details
External job post - https://www.upwork.com/jobs/~010f4ad7e03243ba0c

@parasharrajat
Copy link
Member

Finally, this is landed. App on a whole new level.

@anthony-hull
Copy link
Contributor

anthony-hull commented Aug 9, 2021 via email

@MitchExpensify
Copy link
Contributor

MitchExpensify commented Aug 12, 2021

This is an unintended consequence of this change right?

image

Slack thread

@Luke9389
Copy link
Contributor

Luke9389 commented Aug 12, 2021

I am making a PR to shorten the copy inside the text input. There is a separate regression caused by this PR that's already been identified here.

@Christinadobrzyn Christinadobrzyn changed the title Update form styles on Expensify.cash mobile app [Hold of payment Aug 17th] Update form styles on Expensify.cash mobile app Aug 13, 2021
@Christinadobrzyn
Copy link
Contributor Author

Reopening this because of the regression, I think that's the right next step? @Luke9389 do you know if this should be reopened while the regression is resolved?

@Luke9389
Copy link
Contributor

I'm not sure whether we need to keep this open or not. I think it's functionally pretty much the same. The regression I mentioned above is already being fixed, so the contributor in this issue (@kakajann) doesn't have any actionable steps to take.

@Christinadobrzyn
Copy link
Contributor Author

Paid @kakajann in Upwork! Upwork job closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

15 participants