[Mobile] Video block ui/ux enhancements #15551
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # packages/block-library/src/index.native.js
# Conflicts: # packages/block-library/src/index.native.js
# Conflicts: # packages/block-editor/src/components/media-placeholder/index.native.js # packages/block-library/src/image/edit.native.js
# Conflicts: # packages/block-library/src/image/edit.native.js # packages/block-library/src/video/style.native.scss
pinarol
reviewed
May 17, 2019
| return <SvgIconRetry fill={ styles.iconRetry.fill } />; | ||
| } | ||
|
|
||
| return <SvgIcon fill={ styles.icon.fill } />; |
There was a problem hiding this comment.
Hey @Tug 👋 I updated icon js files to allow injecting props. Tested with web also, don't seem to have a side effect. How does this solution look to you?
There was a problem hiding this comment.
I did a quick test to show what it would look like and pushed here. But after discussing it with you on another channel we agreed that it would be best to handle this separately.
This reverts commit a5bf475.
|
@Tug and I needed to revert the last commit since we had issues on icon sizes. |
pinarol
approved these changes
May 21, 2019
There was a problem hiding this comment.
Tested with WPAndroid, WPiOS and example apps, looks good 👍
we'll handle the icon issue under separate PR wordpress-mobile/gutenberg-mobile#1004
koke
added a commit
that referenced
this pull request
May 28, 2019
In #15551, these were changed to support passing props, but that also means that when you pass the video icon to the `Icon` component, it won't inject the right size, since it's not a SVG component. Instead, we can export the new function to allow for a customizable icon, while exporting the SVG component by default.
5 tasks
hypest
pushed a commit
that referenced
this pull request
May 30, 2019
In #15551, these were changed to support passing props, but that also means that when you pass the video icon to the `Icon` component, it won't inject the right size, since it's not a SVG component. Instead, we can export the new function to allow for a customizable icon, while exporting the SVG component by default.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR improves UI/UX of video and image blocks based on comments from this issue: wordpress-mobile/gutenberg-mobile#966
NOTE
I have implemented all states except one, which is shown in the attached image. I think that we don’t need this state as on Android we have video control which isn’t just Play button, but full video control. So in case of Android, suggested popup would block video control bar + remove block options is presented with trash icon, so we are just doubling action.
TEST
To test from Gutenberg mobile :
Point to branch wordpress-mobile/gutenberg-mobile#979 from WPAndroid or WPiOS application.