-
Notifications
You must be signed in to change notification settings - Fork 58
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
Cross Platform MediaPlaceholder component #1299
Comments
Hey @pinarol , I have some questions regarding Above you've mentioned:
At the moment the
If this is not a problem please mention which props we should unify. Thanks! |
I'm wondering if we are going to support adding a photo by passing the |
We don't support
Right. And the plan is NOT adding a edit.native.js file for it. We are working on making the gallery edit.js file cross platform. And this issue is one of the subitems for that.
As far as I see onSelect passes a media object containing url, id on the web side. On mobile side, it passes the same things but as separate parameters. So we can just wrap those into an object on mobile too. Let me know if I missed sth.
It looks like you'll need to add a native implementation to it. As far as I see BlockIcon is just a styled icon used by Placeholder/MediaPlaceholder on web side. So I'd add a block-icon/index.native.js and style.native.scss for mobile and move the styling from /media-placeholder/styles.native.scss(.modalIcon) to block-icon/style.native.scss. I'll continue answering the rest |
Sure, will handle it! |
This one is interesting because web and mobile display different UI in case of a failed upload. On web it keeps on displaying the MediaPlaceholder with an error message: On mobile it doesn't show the placeholder, it switches to the retry state: And if we tap on a failed upload it shows retry option: So currently, having an "onError" prop on MediaPlaceholder doesn't make sense on mobile because it won't be rendered in case of a failed upload anyway. cc @koke @mkevins this UX difference will be a challenge on x-platform gallery. |
Let me share you this doc to describe what it is. |
I think I got all the questions covered, let me know if I skipped sth. |
@pinarol , thanks for the answers!
I've checked the code and I suppose that on mobile it will add a dashed border and will decrease paddings, but please invite me to Zeplin (my username is |
It looks like BlockIcon will already be added with this PR. Please take a look at that one to avoid conflicts. |
This component uses the MediaUpload component, so it is highly dependent on it.
We already have MediaPlaceholder for mobile and web but they have different props. We need to refactor the mobile implementation(gutenberg/packages/block-editor/src/components/media-placeholder/index.native.js) to work with same props with web.
Note that some props might be web specific and not needed by mobile.
We might also need to update the unittests: gutenberg/packages/block-editor/src/components/media-placeholder/test/index.js
MediaPlaceholder is a simple component that just provides a simple UI when there's no media present:
To test
You can use example app for testing this.
yarn install
yarn start
yarn ios
yarn android
Add image/video blocks and make sure it is keeps displaying the already existing UI for each
Make sure you see media options when you tap on it.
Please also check the MediaPlaceholder usage on gallery block(gutenberg/packages/block-library/src/gallery/edit.js). At the end of this work we expect it will work for mobile without changing it. However we can focus on the usage in the gallery block later and first start with handling image, video blocks.
The text was updated successfully, but these errors were encountered: