-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[RN Mobile] Audio Block: Add from URL - Refactor Prompt component [WIP] #31853
[RN Mobile] Audio Block: Add from URL - Refactor Prompt component [WIP] #31853
Conversation
Size Change: 0 B Total Size: 1.32 MB ℹ️ View Unchanged
|
Hey @ceyhun when you get a chance could you have a look at this approach and share any thoughts you may have? If necessary I can add more screenshots and details if any of the states described aren't clear. Thanks 😄 |
@ceyhun this is not urgent. I know you are focusing on the RN upgrade so your availability is limited right now. @fluiddot I will need your help with reviewing these changes when it is best for you. |
<AddFromUrlBottomSheet | ||
onClose={ ( { url } ) => { | ||
this.setState( { | ||
isAddFromUrlBottomsheetVisible: false, | ||
} ); |
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 not sure I like handling the state for the bottom sheet here. I wanted to encapsulate it within the AddFromUrlBottomSheet
the issue I was having was the AddFromUrlBottomSheet
is a function component while the MediaUpload
is a class component, so the ref ideas I had weren't clear to me. Let me know what you think. This sheet is based on #31343
I will be closing this PR since we were able to get the prompt component working as expected and it is tested and functioning fine on both platforms. |
Draft mode - I will create the GB Mobile PR in short order.
Description
The purpose of this PR is to migrate from the
prompt
component in #27817 to thelink settings
bottom sheet behavior. This move was inspired by the implementation done in Embed Block Bottom Sheet to set a link. The current implementation of the prompt component that creates a polyfill so there can be Android support for theAlert.prompt
API is causing crashes related to Hermes and so far it has been difficult to debug the issue, hence the usage of a component that is already cross-platform.How has this been tested?
Screenshots
Types of changes
Issues Noticed
Some issues have been noticed that may be as a result of how the Link component is built.
Checklist:
*.native.js
files for terms that need renaming or removal).