-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Save button for rate and unit #18454
Save button for rate and unit #18454
Conversation
…calls Signed-off-by: Pierre Michel <pmiche04@gmail.com>
@youssef-lr Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Hello @eVoloshchak this is what I have done for the moment, you can see how that looks like with my web recording. I am going to correct little things here and there but I wonder how you would go about doing what you spoke about with the designer (aligning "Rate" with the text and making sure the hover state touches the border, I am guessing you did this by changing the css directly in chrome: So to get the same result I am guessing that I have to remove the padding of Section and add margin to the texts above "Rate", I wonder how to do that without having ugly code, do you have an idea ? Should I still use Section or just copy it's style ? |
…t, guide management,rate display in the reimburse screen
05dd7d1
to
95e5e00
Compare
Signed-off-by: Pierre Michel <pmiche04@gmail.com>
No need to change the section I think, I just used negative values for margin, passing inputStyle to TextInput |
…kilometer Signed-off-by: Pierre Michel <pmiche04@gmail.com>
Signed-off-by: Pierre Michel <pmiche04@gmail.com> # Conflicts: # src/libs/actions/Policy.js
Signed-off-by: Pierre Michel <pmiche04@gmail.com>
Signed-off-by: Pierre Michel <pmiche04@gmail.com>
Thanks @eVoloshchak , yes negative margin worked. I think everything is good now |
Signed-off-by: Pierre Michel <pmiche04@gmail.com>
@eVoloshchak thanks, I did the suggestions |
Signed-off-by: Pierre Michel <pmiche04@gmail.com>
Signed-off-by: Pierre Michel <pmiche04@gmail.com>
Signed-off-by: Pierre Michel <pmiche04@gmail.com> # Conflicts: # src/ROUTES.js # src/libs/actions/Policy.js # src/pages/workspace/reimburse/WorkspaceReimburseView.js
Signed-off-by: Pierre Michel <pmiche04@gmail.com>
Signed-off-by: Pierre Michel <pmiche04@gmail.com>
Signed-off-by: Pierre Michel <pmiche04@gmail.com>
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-05-23.at.20.25.34.movMobile Web - ChromeScreen_Recording_20230523-203407_Chrome.mp4Mobile Web - SafariScreen.Recording.2023-05-23.at.20.28.18.movDesktopScreen.Recording.2023-05-23.at.20.34.36.moviOSScreen.Recording.2023-05-23.at.20.29.40.movAndroidScreen_Recording_20230523-203243_New.Expensify.mp4 |
@ShogunFire, this looks good, tested well on all platforms
Screen.Recording.2023-05-17.at.17.58.07.mov
|
@youssef-lr all yours. |
@ShogunFire, this branch has conflicts that must be resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@ShogunFire there are conflicts. |
Signed-off-by: Pierre Michel <pmiche04@gmail.com> # Conflicts: # src/libs/actions/Policy.js
df60dc9
@youssef-lr Conflicts solved |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/youssef-lr in version: 1.3.19-0 🚀
|
👋 #19685 (comment) - regression? |
@eVoloshchak :( Looks like there are some changes to do, do you know if there is a way to do only one PR for all those changes ? Or even reopen this PR ? Those are little changes and I don't know if it's worth 3 PR. |
@ShogunFire You can create another PR to fix the regression and pull me for review. |
Looks like there's another regression related to how the field diplays while offline #19702. |
#19704 will also be fixed in a follow-up PR? |
@youssef-lr @eVoloshchak So I have done 3 PR to solve the three bugs but @luacmartins has made a PR that resolves the 3 and some others I think. I thought I was supposed to fix the regressions but I am ok if we take his, less work for me :P |
@ShogunFire Yes, normally you should fix your regressions. I just did it now because it was a deploy blocker linked to it and it was easier to CP that to staging instead of 3 different PRs. I also added an extra fix for it. |
Ok thank you. |
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.19-7 🚀
|
Details
The way to change rate and unit was kind of broken so we decided to regroup the two calls of the API in one and to do this call when we clickon a save button. the rate and unit are now in a different page than the reimburse page.
Fixed Issues
$ 15996
PROPOSAL: 15996 (COMMENT)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
2023-05-05.18-42-12.mp4
Mobile Web - Chrome
2023-05-05.22-32-18.mp4
Mobile Web - Safari
2023-05-05.20-43-58.mp4
Desktop
2023-05-05.20-52-07.mp4
iOS
2023-05-05.20-34-17.mp4
Android
2023-05-05.22-20-24.mp4