Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ensure icons for image/video are SVG components #15863

Merged
merged 1 commit into from
May 30, 2019
Merged

Ensure icons for image/video are SVG components #15863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/block-library/src/image/edit.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import { doAction, hasAction } from '@wordpress/hooks';
*/
import styles from './styles.scss';
import MediaUploadProgress from './media-upload-progress';
import SvgIcon from './icon';
import { SvgIcon } from './icon';
import SvgIconRetry from './icon-retry';

const LINK_DESTINATION_CUSTOM = 'custom';
Expand Down
4 changes: 2 additions & 2 deletions packages/block-library/src/image/icon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
*/
import { Path, SVG } from '@wordpress/components';

function svg( props ) {
export function SvgIcon( props ) {
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( {} );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea is to revert this in #15778, and this is just a temporary fix for this week's mobile release. @hypest should we create a 1.6 branch and merge this there instead of master? We can then revert the icon changes as part of #15778

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@gziolo gziolo May 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically speaking, you can use cloneElement( icon, extraProps ) to inject more props as well.

See: https://reactjs.org/docs/react-api.html#cloneelement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the base branch to rnmobile/release-v1.6.0 and noted the required changes in the upcoming PR: #15778 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could replace that check with extending the SVG component to translate the size prop into width/height

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

2 changes: 1 addition & 1 deletion packages/block-library/src/video/edit.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import { doAction, hasAction } from '@wordpress/hooks';
*/
import MediaUploadProgress from '../image/media-upload-progress';
import style from './style.scss';
import SvgIcon from './icon';
import { SvgIcon } from './icon';
import SvgIconRetry from './icon-retry';

const VIDEO_ASPECT_RATIO = 1.7;
Expand Down
4 changes: 2 additions & 2 deletions packages/block-library/src/video/icon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
*/
import { Path, SVG } from '@wordpress/components';

function svg( props ) {
export function SvgIcon( props ) {
return <SVG viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg" { ...props }><Path fill="none" d="M0 0h24v24H0V0z" /><Path d="M4 6.47L5.76 10H20v8H4V6.47M22 4h-4l2 4h-3l-2-4h-2l2 4h-3l-2-4H8l2 4H7L5 4H4c-1.1 0-1.99.9-1.99 2L2 18c0 1.1.9 2 2 2h16c1.1 0 2-.9 2-2V4z" /></SVG>;
}

export default svg;
export default SvgIcon( {} );