Skip to content

fix/svg image respect width height props on web #2748

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

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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: 2 additions & 0 deletions src/components/icon/icon.api.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
{"name": "assetGroup", "type": "string", "description": "the asset group, default is icons"},
{"name": "tintColor", "type": "string", "description": "The icon tint"},
{"name": "size", "type": "number", "description": "The icon size"},
{"name": "height", "type": "number | string", "description": "The icon height"},
Copy link
Contributor

Choose a reason for hiding this comment

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

also change the type here to number.

{"name": "width", "type": "number | string", "description": "The icon width"},
{"name": "supportRTL", "type": "boolean", "description": "whether the image should flip horizontally on RTL locals"},
{"name": "recorderTag", "type": "'mask' | 'unmask'", "description": "Recorder Tag"}
],
Expand Down
12 changes: 11 additions & 1 deletion src/components/icon/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@ export type IconProps = Omit<ImageProps, 'source'> &
* the icon size
*/
size?: number;
/**
* the icon height
*/
height?: number | string;
Copy link
Contributor

@adids1221 adids1221 Sep 19, 2023

Choose a reason for hiding this comment

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

About the string type, when I pass the string I get this error:

JSON value of type NSString cannot be converted to a YGValue.

To get over this error the user should pass value% as the width or height value and this is not the result we want.

Let's make it only a number type.

git issue

Copy link
Collaborator

@M-i-k-e-l M-i-k-e-l Sep 19, 2023

Choose a reason for hiding this comment

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

@adids1221 what string did you use?
It is working for me with width={'10%'} for example

/**
* the icon width
*/
width?: number | string;
/**
* whether the icon should flip horizontally on RTL
*/
Expand Down Expand Up @@ -53,10 +61,12 @@ const Icon = forwardRef((props: Props, ref: any) => {
assetName,
modifiers,
recorderTag,
width,
height,
...others
} = props;
const {margins} = modifiers;
const iconSize = size ? {width: size, height: size} : undefined;
const iconSize = size ? {width: width ?? size, height: height ?? size} : undefined;
const shouldFlipRTL = supportRTL && Constants.isRTL;

const iconSource = useMemo(() => {
Expand Down
8 changes: 5 additions & 3 deletions src/components/svgImage/index.web.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@ export interface SvgImageProps {
tintColor?: string | null;
data: any; // TODO: I thought this should be string | React.ReactNode but it doesn't work properly
style?: object[];
width?: number | string;
height?: number | string;
}

function SvgImage(props: SvgImageProps) {
const {data, style = [], tintColor, ...others} = props;
const {data, style = [], tintColor, width, height, ...others} = props;
const [id] = useState(`svg-${new Date().getTime().toString()}`);
const [svgStyleCss, setSvgStyleCss] = useState<string | undefined>(undefined);

Expand All @@ -25,13 +27,13 @@ function SvgImage(props: SvgImageProps) {
const {postcss, cssjs} = PostCssPackage;
const styleObj: Record<string, any> = StyleSheet.flatten(style);
postcss()
.process(styleObj, {parser: cssjs})
.process({width, height, ...styleObj}, {parser: cssjs})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add someone that looked at web recently for this part

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.then((style: {css: any}) => {
const svgPathCss = (styleObj?.tintColor) ? `#${id} svg path {fill: ${styleObj?.tintColor}}` : '';
setSvgStyleCss(`#${id} svg {${style.css}} #${id} {${style.css}} ${svgPathCss}}`);
});
}
}, [style, id]);
}, [style, id, width, height]);

if (isSvgUri(data)) {
return <img {...others} src={data.uri} style={StyleSheet.flatten(style)}/>;
Expand Down
8 changes: 6 additions & 2 deletions webDemo/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,14 @@ const itemsToRender: ItemToRender[] = [
size={Button.sizes.large}
iconSource={svgData}
iconStyle={{
width: size,
height: size,
// width: size,
// height: size,
Copy link
Contributor

Choose a reason for hiding this comment

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

If the iconStyle object isn't relevant anymore let's remove it.
Delete the commented lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relevant for the tint.
i'll remove the commented lines

tintColor: '#ffffff'
}}
iconProps={{
width: size,
height: size
}}
onPress={() => {
const newSize = size === Spacings.s4 ? Spacings.s6 : Spacings.s4;
setSize(newSize);
Expand Down