-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix: Cannot paste magic code on mWeb iOS #23254
Fix: Cannot paste magic code on mWeb iOS #23254
Conversation
src/components/MagicCodeInput.js
Outdated
inputStyle={[isMobileSafari ? styles.magicCodeInputTransparent : undefined]} | ||
caretHidden | ||
textInputContainerStyles={[styles.borderNone]} | ||
inputStyle={[styles.magicCodeInputTransparent, isBrowser ? styles.magicCodeInputTransparentWebKit : undefined]} |
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.
The isBrowser
check is to prevent warning since WebkitTextFillColor
prop is only available on Web platforms.
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.
Let's move the isBrowser
check into the styles.js
code?
src/styles/styles.js
Outdated
@@ -2536,6 +2536,9 @@ const styles = { | |||
magicCodeInputTransparent: { |
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.
Ok so my suggestion, is to take the isBrowser
check and put it in styles
and make this name more generic
inputTransparent: {
color: 'transparent',
...(Browser.getBrowser() ? {
caretColor: 'transparent',
...etc
} : {}),
},
Can we do something like this?
@Santhosh-Sellavel 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] |
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.
What's the status here? Can someone summarize and if there are questions let's take one at a time?
@marcaaron We are waiting for this #21708 (comment) |
@Santhosh-Sellavel What do you mean? We are here in the PR right - let's ask some questions? |
Yes @marcaaron, we have the PR here. But we had an ongoing discussion that's waiting for your input on the issue. It's better to discuss it there (As we clear history there) and come back. |
@@ -323,7 +320,7 @@ function MagicCodeInput(props) { | |||
> | |||
<Text style={[styles.magicCodeInput, styles.textAlignCenter]}>{decomposeString(props.value, props.maxLength)[index] || ''}</Text> | |||
</View> | |||
<View style={[StyleSheet.absoluteFillObject, styles.w100, isMobileSafari ? styles.bgTransparent : styles.opacity0]}> | |||
<View style={[StyleSheet.absoluteFillObject, styles.w100, styles.bgTransparent]}> |
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.
Why are we always passing styles.bgTransparent
here? Can you add some comment in the code above this to explain what purpose that serves?
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.
Please add a comment here.
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.
@tienifr bump!
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.
Reading the comment made me wonder why would we need this now in the first place, what does it do?
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.
My understanding is that we're basically doing a "hidden input" trick here.
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.
I'm just curious. Because now we do the trick with styles and props on the TextInput
itself. So I don't understand why would need this, let me know if am missing anything thanks!
src/components/MagicCodeInput.js
Outdated
inputStyle={[isMobileSafari ? styles.magicCodeInputTransparent : undefined]} | ||
caretHidden | ||
textInputContainerStyles={[styles.borderNone]} | ||
inputStyle={[styles.magicCodeInputTransparent, isBrowser ? styles.magicCodeInputTransparentWebKit : undefined]} |
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.
Let's move the isBrowser
check into the styles.js
code?
src/styles/styles.js
Outdated
@@ -2536,6 +2536,9 @@ const styles = { | |||
magicCodeInputTransparent: { |
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.
Ok so my suggestion, is to take the isBrowser
check and put it in styles
and make this name more generic
inputTransparent: {
color: 'transparent',
...(Browser.getBrowser() ? {
caretColor: 'transparent',
...etc
} : {}),
},
Can we do something like this?
That's ok. I specifically asked if we can discuss it here. So let's do that? Thanks. |
@Santhosh-Sellavel @marcaaron I have moved the |
Again this is not something we want, It's better to keep how it was before. Just improve the check. |
Yeah, we can do it here then. We want something like this to eliminate platform-specific checks and make it a reusable style for the future. export function getTransparentStyle() {return isMobileSafari ? styles.bgTransparent : styles.opacity0;} But we can't do that here,
IMO it does make any sense to keep it styles.js or any utils, we can't reuse it anywhere else. Coming to the MagicCodeComponent Here With the help of check
So I think it's better to improve the check of Let me know your thoughts @marcaaron |
Not exactly a requirement if it's not possible. I mainly want that logic out of the component itself and abstracted away a bit.
It might be unlikely that this gets reused. But I'd prefer to have this logic outside of the component. The app philosophy suggests we have platform specific details abstracted away whenever possible.
Let's keep moving. We have to strike a balance between code reuse and cross platform first specific guidance so it's a compromise. |
Bump @Santhosh-Sellavel. Wait for your conclusion. |
@marcaaron Ensuring our expectations are aligned We are just moving forward with refactoring to make the isMobileSafariCheck out side the component, right? |
Reviewer Checklist
Screenshots/VideosMobile Web - SafariSimulator.Screen.Recording.-.iPhone.14.-.2023-08-04.at.00.24.29.mp4iOS ChromeRPReplay_Final1691091661.MP4iOSSimulator.Screen.Recording.-.iPhone.14.-.2023-08-04.at.00.42.08.mp4 |
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.
Looks good and tests well, just waiting for this!
Sorry I've missed that. Please check again. |
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, thanks for the changes.
@@ -323,7 +320,7 @@ function MagicCodeInput(props) { | |||
> | |||
<Text style={[styles.magicCodeInput, styles.textAlignCenter]}>{decomposeString(props.value, props.maxLength)[index] || ''}</Text> | |||
</View> | |||
<View style={[StyleSheet.absoluteFillObject, styles.w100, isMobileSafari ? styles.bgTransparent : styles.opacity0]}> | |||
<View style={[StyleSheet.absoluteFillObject, styles.w100, styles.bgTransparent]}> |
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.
My understanding is that we're basically doing a "hidden input" trick here.
✋ 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/marcaaron in version: 1.3.51-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.51-2 🚀
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.52-0 🚀
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.52-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.52-5 🚀
|
Details
On iOS browsers rather than Safari (Chrome for example), long pressing magic code text input to paste does not work. This PR fixes that.
Fixed Issues
$ #21708
PROPOSAL: #21708 (comment)
Tests
123456
Offline tests
Same above
QA Steps
123456
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
magic-code-web-compressed.mov
Mobile Web - Chrome
Chrome on iOS
original-9FFB921A-303D-4741-B919-178EB9BBE3EC.mp4
Mobile Web - Safari
original-32BAB80E-5506-4CF9-BC4E-8BFEA02FA3E6.mp4
Desktop
magic-code-desktop-compressed.mov
iOS
Screen.Recording.2023-07-21.at.11.41.11.mov
Android
Screen.Recording.2023-07-21.at.11.54.45.mov