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

🍒 Cherry pick PR #36497 to staging 🍒 #36549

Closed
wants to merge 2 commits into from

Conversation

os-botify[bot]
Copy link

@os-botify os-botify bot commented Feb 14, 2024

🍒 Cherry pick #36497 to staging 🍒

OSBotify and others added 2 commits February 14, 2024 23:15
(cherry picked from commit 0381d57)
Fixed currency pressable ui and padding issues

(cherry picked from commit af1026e)
@os-botify os-botify bot requested a review from a team as a code owner February 14, 2024 23:15
Copy link

melvin-bot bot commented Feb 14, 2024

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

@os-botify
Copy link
Author

os-botify bot commented Feb 14, 2024

This pull request has merge conflicts and can not be automatically merged. 😞
Please manually resolve the conflicts, push your changes, and then request another reviewer to review and merge.
Important: There may be conflicts that GitHub is not able to detect, so please carefully review this pull request before approving.

@melvin-bot melvin-bot bot removed the request for review from a team February 14, 2024 23:15
Copy link

melvin-bot bot commented Feb 14, 2024

@cristipaval 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]

@thienlnam
Copy link
Contributor

Okay, this ends up being tricky since on main the file was converted to TS, and then the merge conflict on main ended up conflicting with one of these removed files - but now we're trying to CP this new change which would bring forward those typescript changes which are not yet available on staging.

I don't want to accidently break staging, and looking at the deploy blocker issues it solves, it doesn't look like a high priority so I am just going to let this go through the normal deploy process

cc @amyevans

Comment on lines +53 to +59
<<<<<<< HEAD
{value: amount, currency = CONST.CURRENCY.USD, extraDecimals = 0, errorText, onInputChange, onCurrencyButtonPress}: AmountFormProps,
forwardedRef: ForwardedRef<TextInput>,
=======
{value: amount, currency = CONST.CURRENCY.USD, extraDecimals = 0, errorText, onInputChange, onCurrencyButtonPress, isCurrencyPressable = true}: AmountFormProps,
forwardedRef: ForwardedRef<BaseTextInputRef>,
>>>>>>> af1026e (Merge pull request #36497 from shubham1206agra/fix-currency-ui)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think to solve it you'd just need to copy over the isCurrencyPressable = true param to line 54, but def understand if you don't wanna worry about it and just make the issues NABs

Copy link
Contributor

Choose a reason for hiding this comment

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

That is one thing, but also BaseTextInputRef doesn't exist here yet and then I found a couple other files with missing imports so I didn't want to risk it since I'm not sure what is not on staging yet. I'd have to correct all of those files :yikes:

Copy link
Contributor

Choose a reason for hiding this comment

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

ah okay yeah fair enough! :nevermind:

@amyevans amyevans deleted the thienlnam-cherry-pick-staging-36497-1 branch February 14, 2024 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants