Ensure icons for image/video are SVG components #15863
Conversation
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.
koke
added
the
Mobile App - i.e. Android or iOS
koke
requested review from
ajitbohra,
gziolo,
jorgefilipecosta,
notnownikki,
Soean,
talldan and
youknowriad
as code owners
| return <SVG viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg" { ...props }><Path d="M0,0h24v24H0V0z" fill="none" /><Path d="m19 5v14h-14v-14h14m0-2h-14c-1.1 0-2 0.9-2 2v14c0 1.1 0.9 2 2 2h14c1.1 0 2-0.9 2-2v-14c0-1.1-0.9-2-2-2z" /><Path d="m14.14 11.86l-3 3.87-2.14-2.59-3 3.86h12l-3.86-5.14z" /></SVG>; | ||
| } | ||
|
|
||
| export default svg; | ||
| export default SvgIcon( {} ); |
There was a problem hiding this comment.
I know these icon.js files are supposed to be normalized across all block types. Both the previous and the new notations seem wrong to me (compared to the other files)
cc @gziolo
There was a problem hiding this comment.
In other places, we do something as follows:
return (
<SVG viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg"><Path d="M0,0h24v24H0V0z" fill="none" /><Path d="m19 5v14h-14v-14h14m0-2h-14c-1.1 0-2 0.9-2 2v14c0 1.1 0.9 2 2 2h14c1.1 0 2-0.9 2-2v-14c0-1.1-0.9-2-2-2z" /><Path d="m14.14 11.86l-3 3.87-2.14-2.59-3 3.86h12l-3.86-5.14z" /></SVG>;
);it's an element rather than a component. If that's an issue we can think about ways to update all block icons.
There was a problem hiding this comment.
Technically speaking, you can use cloneElement( icon, extraProps ) to inject more props as well.
There was a problem hiding this comment.
Changed the base branch to rnmobile/release-v1.6.0 and noted the required changes in the upcoming PR: #15778 (comment)
There was a problem hiding this comment.
Technically speaking, you can use cloneElement( icon, extraProps ) to inject more props as well.
I used that API in here and to be honest, it felt kinda awkward, a bit hacky and probably not a well maintainable solution. AFAICT, the Gutenberg code leans more on HOCs to do augmentations on components and we probably should try something similar here? I.e. perhaps the assumptions of the Icon code here are too strict and inflexible?
There was a problem hiding this comment.
I guess we could replace that check with extending the SVG component to translate the size prop into width/height
There was a problem hiding this comment.
I'm going to review approve and merge this PR (to the release branch, not master) if it's working OK. We'll need to work on a separate PR to make it mergable to master.
|
@koke , should we do the same treatment to image/icon-retry and video/icon-retry too? |
|
The retry icons are only used explicitly by the blocks, so there’s no need. The problem was only with the default icon, as the missing block was trying to use that |
|
Not a blocker for this PR but, would it still make sense to unify the implementation to avoid confusion? If yes, we can do it in a separate PR or this one if quick enough. |
|
That's already done in #15778 😁 |
Works for me! |
In #15551, these were changed to support passing props, but that also means
that when you pass the video icon to the
Iconcomponent, it won't inject theright 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.
How has this been tested?
Tested this through wordpress-mobile/gutenberg-mobile#1036 by unregistering the video block, and viewing a post with videos in it.
Screenshots
Before:
After:
Checklist: