-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix: Clear/calendar button is aligned to top instead of in the middle of input modal. #62976
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
Conversation
… of input modal. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
@dominictb 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] |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…may know modal. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@Krishna2323 FYI we can combine all the regressions fixes into 1 PR for better follow-ups. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…ive screen. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@dominictb, all the linked issues are fixed. This one is the only one remaining, which I’ve planned to fix separately. cc: @Expensify/design |
|
Thanks Krishna! @dominictb let's get this into final review when you get a moment, thanks! |
|
🚧 @shawnborton has triggered a test app build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
In my opinion, 1px off center = not a deploy blocker anymore, then we can take our time getting to 0 px off center 😬 thoughts?? |
|
@Beamanator, yes I don't think we should block on that. EDIT: BTW that isn't caused by this or the original PR, it's the icon size due to which we see difference between both icons. Monosnap.screencast.2025-05-29.02-27-21.mp4 |
Definitely agree! Not a deploy blocker! |
|
Any updates @dominictb ? 🙏 |
| return maxAutoGrowHeight ? {maxHeight: maxAutoGrowHeight - styles.textInputMultilineContainer.paddingTop - styles.textInputContainer.borderWidth * 2} : {}; | ||
| }, | ||
|
|
||
| getTextInputIconContainerStyles: (hasLabel: boolean, includePadding = true) => { |
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.
Add a comment here explaining WHY and WHEN we need this; and the params' meaning
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.
added.
src/components/AmountTextInput.tsx
Outdated
| disableKeyboard={disableKeyboard} | ||
| inputStyle={style} | ||
| textInputContainerStyles={[styles.p0, containerStyle]} | ||
| textInputContainerStyles={[containerStyle]} |
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.
| textInputContainerStyles={[containerStyle]} | |
| textInputContainerStyles={containerStyle} |
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.
updated.
| /** Customize the TextInput container */ | ||
| textInputContainerStyles?: StyleProp<ViewStyle>; | ||
|
|
||
| /** Whether to apply padding to the input */ |
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.
Add a comment explain WHY and WHEN we need this prop
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.
updated
| setWidth((prevWidth: number | null) => (autoGrowHeight ? layout.width : prevWidth)); | ||
| setHeight((prevHeight: number) => | ||
| !multiline ? layout.height + heightToFitEmojis - (styles.textInputContainer.padding + styles.textInputContainer.borderWidth * 2) : prevHeight, | ||
| !multiline ? layout.height + heightToFitEmojis - ((hasLabel ? styles.textInputContainer.padding : 0) + styles.textInputContainer.borderWidth * 2) : prevHeight, |
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.
Extract variables for these: labelPadding, borderWidth
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.
done
| {!!inputProps.secureTextEntry && ( | ||
| <Checkbox | ||
| style={[styles.flex1, styles.textInputIconContainer]} | ||
| style={[styles.flex1, StyleUtils.getTextInputIconContainerStyles(hasLabel)]} |
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.
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.
fixed
|
@Krishna2323 are you online to address the PR review? |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@Beamanator, yep! 🙃 I've addressed all the comments. |
|
Wow thanks so much @Krishna2323 ! |
Reviewer Checklist
Screenshots/Videos |
|
@Krishna2323 Text input with currency prefix is misaligned, only on mWeb Chrome. |
|
I've tested everywhere and the above bug should be the last one. |
|
Oofda that's such a small misalignment, i'm tempted to merge 🙈 But i can let this go till morning i guess 😅 (since we have another blocker still open) |
|
Let's try to address this one too: #63010 |
|
Let's fix the misalignment above if we have some time this morning. cc @Krishna2323 @dominictb what's the overall status here? |
|
@Krishna2323 another quick thing - can you check the border color on the input of the reports page? It feels quite light to me, almost as if it's using the wrong value - it should use our theme value for borders: |
|
#63010 was already fixed in this PR. |
|
@Krishna2323 what are the next steps here? Do you plan to make a fresh new PR from your old branch that was reverted? |
|
@shawnborton, the new PR with the reverted changes and the fix for the regressions is here. I'm working on the fix for the last issue, as well as this and this comment. I’ll try to address those within 2 hours so we have a good chance of merging that today. |
|
Sounds good, let's close this one out then. |















































Explanation of Change
Fixed Issues
$ #62931
$ #62947
$ #62971
$ #62969
$ #62982
PROPOSAL:
Tests
/settings/workspaces/AE7DCE11CDAF1CAF/company-cards/vcf/assign-card/transaction-start-dateTest 3
Onboard with private domain email (Domain used should have existing account)
Observe the private domain (People you may know) modal displayed
Verify there is padding between the subtitle & the text inputs in people you may know modal
Test 4
Offline tests
QA Steps
Same as tests.
Verify that no errors appear in the JS console
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop-app.mp4