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 4 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
13 changes: 8 additions & 5 deletions 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,11 +54,13 @@ export function SightCard({ item }: SightCardProps) {
copyPopupRef.current?.open();
};

const getOverlayAttributes = () => ({
style: {
stroke: isDarkTheme ? '#ffffff' : '#000000',
},
});
const getOverlayAttributes: DynamicSVGCustomizationFunctions['getAttributes'] = () => {
return {
arunachalam-monk marked this conversation as resolved.
Show resolved Hide resolved
style: {
stroke: isDarkTheme ? '#ffffff' : '#000000',
},
};
};

return (
<>
Expand Down
4 changes: 2 additions & 2 deletions documentation/src/components/Sights/TopBar/TopBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ import { labels, sights, vehicles } from '@monkvision/sights';
import { SearchBar } from '@site/src/components';
import { SightCard } from '@site/src/components/Sights/SightCard';
import clsx from 'clsx';
import React, { ReactElement, useRef } from 'react';
import React, { FunctionComponentElement, useRef } from 'react';
import styles from './TopBar.module.css';

export interface ListItem {
id: string;
lookups: string[];
component: ReactElement;
component: FunctionComponentElement<typeof SightCard>;
souyahia-monk marked this conversation as resolved.
Show resolved Hide resolved
}

export interface Tab {
Expand Down
9 changes: 6 additions & 3 deletions documentation/src/pages/Sights/index.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { LabelTable, ListItem, Tab, tabs, TopBar } from '@site/src/components';
import { LabelTable, ListItem, SightCard, Tab, tabs, TopBar } from '@site/src/components';
import Layout from '@theme/Layout';
import React, { ReactElement, useMemo, useState } from 'react';
import React, { FunctionComponentElement, useMemo, useState } from 'react';
import styles from './styles.module.css';

function lookupItems(lookup: string, items: ListItem[]): ReactElement[] {
function lookupItems(
lookup: string,
items: ListItem[],
): FunctionComponentElement<typeof SightCard>[] {
const str = lookup.toLowerCase();
const filtered = str
? items.filter((item) => item.lookups.some((lookupStr) => lookupStr.includes(str)))
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): JSX.IntrinsicElements {
souyahia-monk marked this conversation as resolved.
Show resolved Hide resolved
return useMemo(
() =>
element.getAttributeNames().reduce((prev, attr) => {
Expand All @@ -42,8 +42,8 @@ export function useJSXTransformAttributes(element: Element) {
return {
...prev,
[key]: value,
};
}, {}),
} as Partial<JSX.IntrinsicElements>;
souyahia-monk marked this conversation as resolved.
Show resolved Hide resolved
}, {}) as JSX.IntrinsicElements,
souyahia-monk marked this conversation as resolved.
Show resolved Hide resolved
[element],
);
}
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,7 +49,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 getAttributes = jest.fn();

Expand All @@ -66,7 +66,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 +83,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 +102,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 +121,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 +140,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 +158,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 +175,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,46 +7,46 @@ 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);
Expand Down
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