feat(assets): add new "dynamic" illustrations for start screen #3631
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces a new React component called Changes
Sequence Diagram(s)sequenceDiagram
participant UI as SVGAsImage Component
participant Hook as useSVGAsImage Hook
participant Fetch as Network/Fetch
participant Util as getCSSPropertyValue Utility
UI->>Hook: Provide SVG source via props
Hook->>Fetch: Fetch SVG content
Fetch-->>Hook: Return raw SVG data
Hook->>Util: Substitute CSS variable references in SVG
Util-->>Hook: Return processed SVG text
Hook->>Hook: Encode SVG text to base64
Hook-->>UI: Return encoded SVG data
UI->>UI: Render <img> element with encoded src
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/components/svg_as_image/index.tsx (1)
12-16: Optimize props handling for better performance.While the implementation is correct, we can optimize the props destructuring:
-const SVGAsImage = (props: SVGAsImageProps) => { - const { encodedSVG } = useSVGAsImage({ src: props.src }); - - return <img src={encodedSVG} alt={props.alt} style={props.style} />; +const SVGAsImage = ({ src, alt, style, ...rest }: SVGAsImageProps) => { + const { encodedSVG } = useSVGAsImage({ src }); + + return <img src={encodedSVG} alt={alt} style={style} {...rest} />;This change:
- Destructures props at the parameter level
- Spreads remaining props to support additional img attributes
- Reduces prop access overhead
src/components/svg_as_image/useSVGAsImage.tsx (1)
11-33: Implement caching mechanism for performance optimization.The current implementation fetches and converts the SVG on every mount and theme change. Consider implementing a caching mechanism.
+const svgCache = new Map<string, string>(); + const convertSvgToBase64 = async (svgUrl: string): Promise<string> => { try { + const cacheKey = `${svgUrl}-${theme}`; + if (svgCache.has(cacheKey)) { + return svgCache.get(cacheKey)!; + } + const response = await fetch(svgUrl); let svgText = await response.text(); const regex = /var\(--([^)]*)\)/g; let match; while ((match = regex.exec(svgText)) !== null) { const varName = match[1]; const value = getCSSPropertyValue(`--${varName}`); svgText = svgText.replace(match[0], value); } const encodedSvg = encodeURIComponent(svgText) .replace(/'/g, '%27') .replace(/"/g, '%22'); - return `data:image/svg+xml;charset=utf-8,${encodedSvg}`; + const result = `data:image/svg+xml;charset=utf-8,${encodedSvg}`; + svgCache.set(cacheKey, result); + return result; } catch (error) { console.error('Error fetching or converting SVG:', error); + setError(error instanceof Error ? error : new Error('Failed to convert SVG')); return ''; } };src/features/app_start/shared/illustration/index.tsx (1)
72-75: Handle loading states and implement lazy loading.The SVGAsImage implementation should handle loading states and implement lazy loading for better performance.
<SVGAsImage src={slide.src} - style={{ width: '100%', height: 'auto' }} + style={{ + width: '100%', + height: 'auto', + opacity: isLoading ? 0.5 : 1, + transition: 'opacity 0.2s ease-in-out' + }} + loading="lazy" />Also, consider handling the loading and error states:
const { encodedSVG, isLoading, error } = useSVGAsImage({ src: slide.src }); if (error) { console.error('Failed to load illustration:', error); // Render fallback illustration }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
src/assets/img/illustration_meetingSchedules.svgis excluded by!**/*.svg,!**/*.svgsrc/assets/img/illustration_ministryAssignments.svgis excluded by!**/*.svg,!**/*.svgsrc/assets/img/illustration_multiPlattform.svgis excluded by!**/*.svg,!**/*.svgsrc/assets/img/illustration_otherSchedules.svgis excluded by!**/*.svg,!**/*.svgsrc/assets/img/illustration_secretary.svgis excluded by!**/*.svg,!**/*.svgsrc/assets/img/illustration_territories.svgis excluded by!**/*.svg,!**/*.svg
📒 Files selected for processing (6)
src/components/svg_as_image/index.tsx(1 hunks)src/components/svg_as_image/index.types.ts(1 hunks)src/components/svg_as_image/useSVGAsImage.tsx(1 hunks)src/features/app_start/shared/illustration/index.tsx(3 hunks)src/features/app_start/shared/illustration/useIllustration.tsx(2 hunks)src/features/meetings/my_assignments/index.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/features/app_start/shared/illustration/useIllustration.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Code QL
- GitHub Check: Summary
🔇 Additional comments (3)
src/components/svg_as_image/index.types.ts (1)
1-6: LGTM! Well-structured type definition.The type definition correctly extends React's built-in types for HTML image elements, providing comprehensive type safety for the SVGAsImage component.
src/features/meetings/my_assignments/index.tsx (2)
12-13: LGTM! Import changes align with new SVG handling approach.The shift from
?componentto?urland addition ofSVGAsImagealigns with the PR's objective of standardizing dynamic illustration handling.
85-85: LGTM! SVG rendering updated with consistent dimensions.The implementation properly uses the new
SVGAsImagecomponent with explicit width and height, ensuring consistent rendering.
|
Hi @FussuChalice: please could you explain a little bit the purpose of this PR? I failed to get it correctly on first look. Thank you. |
Added new "dynamic" illustrations for the start screen. The SVG illustrations now use CSS variables, allowing them to change automatically when the theme is updated. For more details, you can check with Vlad. |
|
@FussuChalice: Yes, I understand that this part of the ClickUp task linked above, but the I do not fully understand the purpose of the changes here. Like in the already working No Notification icon, we just simply import the svg file as a component, and that’s already enough. So that’s why I am wondering do we really this need change, or is there an issue with the current approach that require this new one? Thanks. |
Sorry, that's a typo, I meant NoAssigmentsImg. |
@rhahao this should be fixed with this PR: |
rhahao
force-pushed
the
CU-86c0wjqg8_FEAT-Link-color-variables-of-SVG-illustrations_Max-Makaluk
branch
from
334ba31 to
ef81feb
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/svg_as_image/index.tsx (1)
4-11: Enhance component documentation with additional details.While the documentation follows JSDoc conventions, consider adding:
- Expected format/requirements for the SVG source
- Loading state handling
- Error scenarios and their handling
/** * A React component that converts an SVG source into an encoded image and renders it. * + * @remarks + * The SVG source should be a valid SVG file accessible via URL or file path. + * The component will show nothing while the SVG is being loaded. + * If the SVG fails to load or encode, an error will be thrown. * * @component * @param {SVGAsImageProps} props - The properties for the component. + * @param {string} props.src - The source URL or path to the SVG file + * @param {string} props.alt - Alternative text for accessibility + * @param {React.CSSProperties} [props.style] - Optional CSS styles to apply * * @returns {JSX.Element} The rendered image element. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
src/assets/img/illustration_meetingSchedules.svgis excluded by!**/*.svg,!**/*.svgsrc/assets/img/illustration_ministryAssignments.svgis excluded by!**/*.svg,!**/*.svgsrc/assets/img/illustration_multiPlattform.svgis excluded by!**/*.svg,!**/*.svgsrc/assets/img/illustration_no_assigments.svgis excluded by!**/*.svg,!**/*.svgsrc/assets/img/illustration_otherSchedules.svgis excluded by!**/*.svg,!**/*.svgsrc/assets/img/illustration_secretary.svgis excluded by!**/*.svg,!**/*.svgsrc/assets/img/illustration_territories.svgis excluded by!**/*.svg,!**/*.svg
📒 Files selected for processing (5)
src/components/svg_as_image/index.tsx(1 hunks)src/components/svg_as_image/index.types.ts(1 hunks)src/components/svg_as_image/useSVGAsImage.tsx(1 hunks)src/features/app_start/shared/illustration/index.tsx(3 hunks)src/features/app_start/shared/illustration/useIllustration.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/svg_as_image/index.types.ts
- src/features/app_start/shared/illustration/useIllustration.tsx
- src/features/app_start/shared/illustration/index.tsx
- src/components/svg_as_image/useSVGAsImage.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Code QL
- GitHub Check: Summary
🔇 Additional comments (3)
src/components/svg_as_image/index.tsx (3)
1-2: LGTM! Clean imports with good separation of concerns.The imports are well-organized, with types properly separated into their own file.
18-18: LGTM! Clean export.The export follows React conventions.
1-18:❓ Verification inconclusive
Verify dynamic theming and size constraints.
Based on the PR objectives:
- Ensure the
useSVGAsImagehook properly handles CSS variables for theme changes- Verify that the component respects size constraints to prevent out-of-bounds issues
Let's verify the implementation:
🏁 Script executed:
#!/bin/bash # Check if the useSVGAsImage hook handles CSS variables ast-grep --pattern 'function useSVGAsImage($props) { $$$ // Look for CSS variable handling $$$ }' # Check for size constraints in the StartupIllustration component rg -A 5 'StartupIllustration.*style'Length of output: 144
Double-check dynamic theming in the SVG image and ensure image sizing prevents overflow.
Based on the PR objectives, please verify the following:
Dynamic Theming:
Confirm that theuseSVGAsImagehook correctly interprets CSS variables (or equivalent theme parameters) so that theme changes automatically update the SVG illustration.Size Constraints:
Ensure that any rendered images (including usage within components like StartupIllustration) apply size constraints to avert out-of-bounds rendering.Note: Automated searches for CSS variable handling and references to a StartupIllustration component returned no conclusive evidence. Please manually verify that:
- The underlying hook (
useSVGAsImage) implements theme-aware logic (e.g., using CSS custom properties or similar techniques).- Image sizing is managed appropriately either within this component or via its parent component(s).
| const SVGAsImage = (props: SVGAsImageProps) => { | ||
| const { encodedSVG } = useSVGAsImage({ src: props.src }); | ||
|
|
||
| return <img src={encodedSVG} alt={props.alt} style={props.style} />; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and performance optimizations.
The component needs additional robustness and performance improvements:
- Add loading state handling
- Implement error handling
- Optimize with memo for large SVGs
- Handle undefined props safely
-const SVGAsImage = (props: SVGAsImageProps) => {
- const { encodedSVG } = useSVGAsImage({ src: props.src });
+const SVGAsImage = React.memo((props: SVGAsImageProps) => {
+ const { encodedSVG, isLoading, error } = useSVGAsImage({ src: props.src });
+
+ if (error) {
+ console.error('Failed to load SVG:', error);
+ return null; // or fallback image
+ }
+
+ if (isLoading) {
+ return null; // or loading spinner
+ }
- return <img src={encodedSVG} alt={props.alt} style={props.style} />;
+ return (
+ <img
+ src={encodedSVG}
+ alt={props.alt ?? ''}
+ style={props.style}
+ onError={(e) => {
+ console.error('Failed to render encoded SVG');
+ e.currentTarget.style.display = 'none';
+ }}
+ />
+ );
-};
+});
+
+SVGAsImage.displayName = 'SVGAsImage';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const SVGAsImage = (props: SVGAsImageProps) => { | |
| const { encodedSVG } = useSVGAsImage({ src: props.src }); | |
| return <img src={encodedSVG} alt={props.alt} style={props.style} />; | |
| }; | |
| const SVGAsImage = React.memo((props: SVGAsImageProps) => { | |
| const { encodedSVG, isLoading, error } = useSVGAsImage({ src: props.src }); | |
| if (error) { | |
| console.error('Failed to load SVG:', error); | |
| return null; // or fallback image | |
| } | |
| if (isLoading) { | |
| return null; // or loading spinner | |
| } | |
| return ( | |
| <img | |
| src={encodedSVG} | |
| alt={props.alt ?? ''} | |
| style={props.style} | |
| onError={(e) => { | |
| console.error('Failed to render encoded SVG'); | |
| e.currentTarget.style.display = 'none'; | |
| }} | |
| /> | |
| ); | |
| }); | |
| SVGAsImage.displayName = 'SVGAsImage'; |
|
@FussuChalice, @ux-git : The clipped image above can be fixed by simply adjusting the About the approach of dynamically changing the colors for the startup illustration, the current approach in this PR works, but I think it’s too complex and heavy. So I would suggest creating standalone SVG components for each illustration using the the SvgIcon from MUI, a similar method we use for all icons. We can also fine-tune and fix some colors assigned to some illustrations. Thanks. |
Checklist: