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.

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

🐛 svg styles override unintentionally #783

Merged
merged 6 commits into from
Jun 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
3 changes: 2 additions & 1 deletion documentation/src/components/Sights/SightCard/SightCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { useColorMode } from '@docusaurus/theme-common';
import { labels, sights } from '@monkvision/sights';
import { Sight, VehicleDetails, VehicleModel } from '@monkvision/types';
import { CopyPopup, CopyPopupHandle } from '@site/src/components';
import { DynamicSVGCustomizationFunctions } from '@monkvision/common-ui-web';
import { DynamicSVG } from '../../domOnly';
import styles from './SightCard.module.css';

Expand Down Expand Up @@ -53,7 +54,7 @@ export function SightCard({ item }: SightCardProps) {
copyPopupRef.current?.open();
};

const getOverlayAttributes = () => ({
const getOverlayAttributes: DynamicSVGCustomizationFunctions['getAttributes'] = () => ({
style: {
stroke: isDarkTheme ? '#ffffff' : '#000000',
},
Expand Down
3 changes: 1 addition & 2 deletions documentation/src/components/Sights/TopBar/TopBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ export interface TopBarProps {
onSelectTab: (tab: Tab) => void;
onLookupInput: (value: string) => void;
}

export const tabs = {
export const tabs: Record<'sights' | 'vehicles' | 'labels', Tab> = {
souyahia-monk marked this conversation as resolved.
Show resolved Hide resolved
sights: {
key: 'sights',
items: Object.values(sights).map((sight) => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export interface DynamicSVGProps
export function DynamicSVG({ svg, ...passThroughProps }: DynamicSVGProps) {
const doc = useXMLParser(svg);
const svgEl = useMemo(() => {
const element = doc.children[0];
const element = doc.children[0] as SVGSVGElement;
if (element.tagName !== 'svg') {
throw new Error(
`Invalid SVG string provided to the DynamicSVG component: expected <svg> tag as the first children of XML document but got <${element.tagName}>.`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,12 @@ export function SVGElement<T extends keyof JSXIntrinsicSVGElements = 'svg'>({
}: SVGElementProps<T>) {
const Tag = element.tagName as T;
const attributes = useJSXTransformAttributes(element);
const customAttributes = useCustomAttributes({ element, groupIds, getAttributes });
const customAttributes = useCustomAttributes({
element,
groupIds,
getAttributes,
style: attributes.style ?? {},
});
const tagAttr = { ...attributes, ...customAttributes, ...passThroughProps } as any;
const innerHTML = useInnerHTML({ element, groupIds, getInnerText });
const childrenGroupIds = getChildrenGroupIds({ element, groupIds });
Expand All @@ -53,7 +58,7 @@ export function SVGElement<T extends keyof JSXIntrinsicSVGElements = 'svg'>({
...Array.from(element.children).map((child, id) => (
<SVGElement
key={id.toString()}
element={child}
element={child as SVGSVGElement}
groupIds={childrenGroupIds}
getAttributes={getAttributes}
getInnerText={getInnerText}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export interface DynamicSVGCustomizationFunctions {
* @return This callback should return a set of custom HTML attributes to pass to the element or `null` for no
* attributes.
*/
getAttributes?: (element: Element, groupIds: string[]) => SVGProps<SVGSVGElement> | null;
getAttributes?: (element: SVGElement, groupIds: string[]) => SVGProps<SVGElement>;
/**
* A callback used to customize the inner text of SVG tags in a DynamicSVG component based on the HTMLElement itself,
* or the IDs of the groups this element is part of.
Expand All @@ -23,7 +23,7 @@ export interface DynamicSVGCustomizationFunctions {
* @param groupIds The IDs of the SVG group elements this element is part of (the IDs are in order).
* @return This callback should return a string to use for the innerText of the element or `null` for no innerText.
*/
getInnerText?: (element: Element, groupIds: string[]) => string | null;
getInnerText?: (element: SVGSVGElement, groupIds: string[]) => string | null;
}

/**
Expand All @@ -34,7 +34,7 @@ export interface SVGElementCustomProps {
/**
* The HTMLElement representing the SVG tag that the SVGElement is supposed to display.
*/
element: Element;
element: SVGSVGElement;
/**
* The IDs of the SVG groups this element is part of (in order).
*
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { SVGProps, useMemo } from 'react';
import { CSSProperties, SVGProps, useMemo } from 'react';
import { DynamicSVGCustomizationFunctions, SVGElementCustomProps } from './types';

/**
Expand All @@ -10,15 +10,16 @@ export function useCustomAttributes({
element,
groupIds,
getAttributes,
style,
}: Pick<DynamicSVGCustomizationFunctions, 'getAttributes'> &
Required<SVGElementCustomProps>): SVGProps<SVGSVGElement> | null {
Required<SVGElementCustomProps> & { style: CSSProperties }): SVGProps<SVGElement> | null {
return useMemo(() => {
const elementTag = element.tagName;

if (['svg', 'g'].includes(elementTag)) {
return { pointerEvents: 'box-none' };
}

return getAttributes ? getAttributes(element, groupIds) : null;
if (!getAttributes) return { style };
const attributes = getAttributes(element, groupIds);
return { ...attributes, style: { ...attributes?.style, ...style } };
}, [element, groupIds, getAttributes]);
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ function tranformJsxAttribute(attr: HtmlAttribute): JsxAttribute {
* Custom hook used to map HTML attributes to their JSX counterpart (ex: "class" becomes "className", properly mapping
* inline style values etc.).
*/
export function useJSXTransformAttributes(element: Element) {
export function useJSXTransformAttributes(element: Element): Partial<JSX.IntrinsicElements> {
return useMemo(
() =>
element.getAttributeNames().reduce((prev, attr) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe('SVGElement component', () => {
tagName: 'svg',
children: [],
getAttribute: (a: string) => a,
} as unknown as Element;
} as unknown as SVGSVGElement;

const { unmount, container } = render(<SVGElement element={element} />);

Expand All @@ -36,7 +36,7 @@ describe('SVGElement component', () => {
tagName: 'svg',
children: [],
getAttribute: (a: string) => a,
} as unknown as Element;
} as unknown as SVGSVGElement;

const { unmount } = render(<SVGElement element={element} />);

Expand All @@ -49,15 +49,20 @@ describe('SVGElement component', () => {
tagName: 'svg',
children: [],
getAttribute: (a: string) => a,
} as unknown as Element;
} as unknown as SVGSVGElement;
const groupIds = ['test-id', 'test-id-23'];
const getAttributes = jest.fn();

const { unmount } = render(
<SVGElement element={element} groupIds={groupIds} getAttributes={getAttributes} />,
);

expect(useCustomAttributes).toHaveBeenCalledWith({ element, groupIds, getAttributes });
expect(useCustomAttributes).toHaveBeenCalledWith({
element,
groupIds,
getAttributes,
style: {},
});
unmount();
});

Expand All @@ -66,7 +71,7 @@ describe('SVGElement component', () => {
tagName: 'svg',
children: [],
getAttribute: (a: string) => a,
} as unknown as Element;
} as unknown as SVGSVGElement;
const groupIds = ['test-id', 'test-id-23'];
const getInnerText = jest.fn();

Expand All @@ -83,7 +88,7 @@ describe('SVGElement component', () => {
tagName: 'svg',
children: [],
getAttribute: (a: string) => a,
} as unknown as Element;
} as unknown as SVGSVGElement;

const { unmount, container } = render(<SVGElement element={element} />);

Expand All @@ -102,7 +107,7 @@ describe('SVGElement component', () => {
tagName: 'svg',
children: [],
getAttribute: (a: string) => a,
} as unknown as Element;
} as unknown as SVGSVGElement;

const { unmount, container } = render(<SVGElement element={element} />);

Expand All @@ -121,7 +126,7 @@ describe('SVGElement component', () => {
tagName: 'svg',
children: [],
getAttribute: (a: string) => a,
} as unknown as Element;
} as unknown as SVGSVGElement;
const props: Record<string, string> = { id: 'test-id', from: 'test-from' };

const { unmount, container } = render(<SVGElement element={element} {...props} />);
Expand All @@ -140,7 +145,7 @@ describe('SVGElement component', () => {
tagName: 'svg',
children: [],
getAttribute: (a: string) => a,
} as unknown as Element;
} as unknown as SVGSVGElement;

const { unmount, container } = render(<SVGElement element={element} />);

Expand All @@ -158,7 +163,7 @@ describe('SVGElement component', () => {
{ tagName: 'path', children: [], getAttribute: (a: string) => a },
],
getAttribute: (a: string) => a,
} as unknown as Element;
} as unknown as SVGSVGElement;

const { unmount, container } = render(<SVGElement element={element} />);

Expand All @@ -175,7 +180,7 @@ describe('SVGElement component', () => {
tagName: 'svg',
children: [{ tagName: 'g', children: [], getAttribute: (a: string) => a }],
getAttribute: (a: string) => a,
} as unknown as Element;
} as unknown as SVGSVGElement;
const getAttributes = jest.fn();
const getInnerText = jest.fn();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,50 +7,64 @@ describe('useCustomAttributes hook', () => {
});

it('should remove the pointer events of svg tags', () => {
const element = { tagName: 'svg' } as unknown as Element;
const element = { tagName: 'svg' } as unknown as SVGSVGElement;

const { result, unmount } = renderHook(useCustomAttributes, {
initialProps: { element, groupIds: [] },
initialProps: { element, groupIds: [], style: {} },
});

expect(result.current).toEqual({ pointerEvents: 'box-none' });
unmount();
});

it('should remove the pointer events of g tags', () => {
const element = { tagName: 'g' } as unknown as Element;
const element = { tagName: 'g' } as unknown as SVGSVGElement;

const { result, unmount } = renderHook(useCustomAttributes, {
initialProps: { element, groupIds: [] },
initialProps: { element, groupIds: [], style: {} },
});

expect(result.current).toEqual({ pointerEvents: 'box-none' });
unmount();
});

it('should return null if no customization function is passed', () => {
const element = { tagName: 'path' } as unknown as Element;
const element = { tagName: 'path' } as unknown as SVGSVGElement;

const { result, unmount } = renderHook(useCustomAttributes, {
initialProps: { element, groupIds: [] },
initialProps: { element, groupIds: [], style: {} },
});

expect(result.current).toBeNull();
expect(result.current).toEqual({ style: {} });
unmount();
});

it('should return the result of the customization function', () => {
const element = { tagName: 'path' } as unknown as Element;
const element = { tagName: 'path' } as unknown as SVGSVGElement;
const groupIds = ['test-id-1', 'test-id-2'];
const customAttr = { style: { color: 'test-color' } };
const getAttributes = jest.fn(() => customAttr);

const { result, unmount } = renderHook(useCustomAttributes, {
initialProps: { element, groupIds, getAttributes },
initialProps: { element, groupIds, getAttributes, style: customAttr.style },
});

expect(getAttributes).toHaveBeenCalledWith(element, groupIds);
expect(result.current).toEqual(customAttr);
unmount();
});

it('should apply style correctly', () => {
souyahia-monk marked this conversation as resolved.
Show resolved Hide resolved
const elementStyle = { stroke: '#fff' },
customStyle = { color: 'test-color' };
const element = { tagName: 'path', style: { stroke: '#fff' } } as SVGSVGElement;
const groupIds = ['test-id-1', 'test-id-2'];
const getAttributes = jest.fn(() => ({ style: customStyle }));
const { result, unmount } = renderHook(useCustomAttributes, {
initialProps: { element, groupIds, getAttributes, style: elementStyle },
});
expect(getAttributes).toHaveBeenCalledWith(element, groupIds);
expect(result.current).toEqual({ style: { ...elementStyle, ...customStyle } });
unmount();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ describe('useInnerHTML hook', () => {
});

it('should return the element innerHTML if it is a style tag', () => {
const element = { tagName: 'style', innerHTML: 'test-html' } as unknown as Element;
const element = { tagName: 'style', innerHTML: 'test-html' } as unknown as SVGSVGElement;

const { result, unmount } = renderHook(useInnerHTML, {
initialProps: { element, groupIds: [] },
Expand All @@ -18,7 +18,7 @@ describe('useInnerHTML hook', () => {
});

it('should return null if no customization function is passed', () => {
const element = { tagName: 'path' } as unknown as Element;
const element = { tagName: 'path' } as unknown as SVGSVGElement;

const { result, unmount } = renderHook(useInnerHTML, {
initialProps: { element, groupIds: [] },
Expand All @@ -29,13 +29,13 @@ describe('useInnerHTML hook', () => {
});

it('should return the result of the customization function', () => {
const element = { innerHTML: 'test-html' } as unknown as Element;
const element = { innerHTML: 'test-html' } as unknown as SVGSVGElement;
const groupIds = ['test-id-1', 'test-id-2'];
const innerText = 'inner-text-test';
const getInnerText = jest.fn(() => innerText);

const { result, unmount } = renderHook(useInnerHTML, {
initialProps: { element, groupIds, getInnerText },
initialProps: { element, groupIds, getInnerText, style: {} },
});

expect(getInnerText).toHaveBeenCalledWith(element, groupIds);
Expand Down