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.

Sign up for GitHub

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

Jump to bottom

[No QA] VBA Requestor Step (UI Only) #3582

Merged
merged 6 commits into from
Jun 17, 2021
Merged

[No QA] VBA Requestor Step (UI Only) #3582

merged 6 commits into from
Jun 17, 2021

Conversation

NikkiWines
Copy link
Contributor

@NikkiWines NikkiWines commented Jun 14, 2021 •
edited
Loading

Details

Implements the UI portion of the VBA Requestor Step.

Fixed Issues

Fixes (partially) https://github.com/Expensify/Expensify/issues/166919

Tests / QA Steps

  1. In ReimbursementAccountPage.js, apply the following diff:
-  const currentStep = this.getStepToOpenFromRouteParams() || this.props.reimbursementAccount.achData.currentStep;
+  const currentStep = CONST.BANK_ACCOUNT.STEP.REQUESTOR;
  1. Go to http://localhost:8080/bank-account/
  2. Look at the Requestor Information form and confirm it looks all great 🌟

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-06-16 at 5 00 22 PM

Screen Shot 2021-06-16 at 5 00 29 PM

Mobile Web

Desktop

iOS

Android

@NikkiWines NikkiWines self-assigned this Jun 14, 2021
@NikkiWines NikkiWines changed the base branch from main to marcaaron-setupWithdrawlAccount June 14, 2021 19:55
Base automatically changed from marcaaron-setupWithdrawlAccount to main June 15, 2021 18:46
@NikkiWines NikkiWines changed the base branch from main to Rory-CompanyStep June 16, 2021 21:45
Base automatically changed from Rory-CompanyStep to main June 16, 2021 23:08
@NikkiWines NikkiWines changed the title [WIP] Add VBA Requestor Step VBA Requestor Step (UI Only) Jun 16, 2021
@NikkiWines
update RequestorStep to take advantage of new consts
8be2c4a 
modify it to be stateless for now
@NikkiWines NikkiWines marked this pull request as ready for review June 17, 2021 00:07
@NikkiWines NikkiWines requested a review from a team as a code owner June 17, 2021 00:07
@MelvinBot MelvinBot requested review from luacmartins and removed request for a team June 17, 2021 00:08
@NikkiWines NikkiWines changed the title VBA Requestor Step (UI Only) [No QA] VBA Requestor Step (UI Only) Jun 17, 2021
roryabraham
roryabraham approved these changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB but the links cause a line break on mWeb. Not sure if that's expected.

image

Otherwise this is working great!

marcaaron
marcaaron approved these changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LooksGreatToMe just 1 small comment.

src/pages/ReimbursementAccount/RequestorStep.js

const RequestorStep = () => <View />;
export default RequestorStep;
const RequestorStep = ({translate}) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB, this is going to require state to hold all the form values so we should make it a class component to start + hook that stuff up, but not submit the form yet (or at least just make it a class component since it will have state). Could also wait to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, yeah if you look at prior commits it was not a stateless component in earlier versions - I updated it to match Rory's PR since that how we merged it.

I'm happy to update it in a future PR when we hook this up to the API

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouchies 🙃

luacmartins
luacmartins approved these changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🥇

@luacmartins
Copy link
Contributor

Looks like there is a NAB from @marcaaron, so @NikkiWines I'll leave the decision to you to address that or self merge the PR.

@NikkiWines NikkiWines merged commit 5394fc8 into main Jun 17, 2021
@NikkiWines NikkiWines deleted the nikki-requestorStep branch June 17, 2021 01:23
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.70-1🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify OSBotify mentioned this pull request Jun 17, 2021
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.73-3🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcaaron marcaaron marcaaron approved these changes

@luacmartins luacmartins luacmartins approved these changes

@roryabraham roryabraham roryabraham approved these changes

Assignees

@NikkiWines NikkiWines

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@NikkiWines @luacmartins @OSBotify @marcaaron @roryabraham