feat(app_start): add dynamic illustrations for appStartPage #3679
Conversation
…ions_Max-Makaluk' of /~https://github.com/FussuChalice/organized-app into CU-86c0wjqg8_FEAT-Link-color-variables-of-SVG-illustrations_Max-Makaluk
…G-illustrations_Max-Makaluk
…G-illustrations_Max-Makaluk
…ions_Max-Makaluk' of /~https://github.com/FussuChalice/organized-app into CU-86c0wjqg8_FEAT-Link-color-variables-of-SVG-illustrations_Max-Makaluk
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces multiple new React functional components for SVG illustrations that utilize Material-UI's Changes
Sequence Diagram(s)sequenceDiagram
participant S as StartupIllustration
participant U as useIllustration Hook
participant I as Illustration Component
S->>U: Request slides array
U-->>S: Return slides with `component` property
S->>I: Render selected slide.component
I-->>S: Return rendered SVG illustration
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 0
🧹 Nitpick comments (7)
src/components/svg_as_image/index.tsx (1)
12-16: Consider forwarding all props to the img element.The component only passes
src,alt, andstyleprops to the img element. Consider using prop spreading to forward all valid HTML attributes to ensure the component is fully extensible.- return <img src={encodedSVG} alt={props.alt} style={props.style} />; + return <img src={encodedSVG} {...props} />;Note: You may need to exclude the
srcprop from the spread since you're replacing it withencodedSVG.src/components/illustrations/IllustrationMeetingSchedules.tsx (1)
5-341: Consider optimizing the SVG for better performance.The SVG content is quite large and complex. While it looks comprehensive, consider optimizing it to reduce file size and improve rendering performance:
- Use an SVG optimizer tool (SVGO)
- Remove unnecessary IDs, metadata, and attributes
- Simplify paths where possible
src/components/svg_as_image/useSVGAsImage.tsx (2)
6-49: Add cancellation handling for asynchronous operationsThe hook correctly triggers fetching and conversion when dependencies change, but lacks a cleanup mechanism if the component unmounts before the fetch operation completes.
Add an abort controller to prevent potential memory leaks:
useEffect(() => { + const controller = new AbortController(); const fetchAndConvert = async () => { const base64 = await convertSvgToBase64(src); setEncodedSVG(base64); }; fetchAndConvert(); + + return () => { + controller.abort(); + }; }, [src, theme]);Then update the fetch call to use it:
const convertSvgToBase64 = async (svgUrl: string): Promise<string> => { try { - const response = await fetch(svgUrl); + const response = await fetch(svgUrl, { signal: controller.signal }); let svgText = await response.text(); // Rest of the function remains unchanged
15-22: Performance optimization for SVG processingThe regex processing might be inefficient for large SVGs with many CSS variables.
Consider adding an early check to avoid processing if no CSS variables are present:
try { const response = await fetch(svgUrl); let svgText = await response.text(); const regex = /var\(--([^)]*)\)/g; + + // Quick check if CSS variables exist before processing + if (!svgText.includes('var(--')) { + const encodedSvg = encodeURIComponent(svgText) + .replace(/'/g, '%27') + .replace(/"/g, '%22'); + return `data:image/svg+xml;charset=utf-8,${encodedSvg}`; + } + let match; while ((match = regex.exec(svgText)) !== null) { // Processing logic }src/features/app_start/shared/illustration/useIllustration.tsx (3)
38-43: Extract common style to reduce duplicationAll illustration components use the same style object, which introduces repetition in the code.
Create a shared style object to reduce duplication:
+const illustrationStyle = { width: '100%', height: 'auto' }; const slides = useMemo(() => { return [ { title: t('tr_illustrationMinistryAssignmentsHeader'), desc: t('tr_illustrationMinistryAssignmentsDescription'), component: ( <IllustrationMinistryAssignments - style={{ width: '100%', height: 'auto' }} + style={illustrationStyle} /> ), }, // Apply the same pattern to other illustration componentsAlso applies to: 47-52, 56-61, 65-67, 72-76
78-82: Consider removing commented code for TerritoriesThe commented-out Territories entry adds noise to the codebase without providing value. Either remove it entirely if no longer needed or add a comment explaining why it's kept.
- // { - // title: t('tr_illustrationTerritoriesHeader'), - // desc: t('tr_illustrationTerritoriesDescription'), - // src: Territories, - // },
86-90: Add type annotation for function parameterThe
handleSlidefunction parameternlacks a type annotation, which could lead to type-related issues.- const handleSlide = (n) => { + const handleSlide = (n: number) => { if (swiperRef.current) { swiperRef.current.swiper.slideToLoop(n); } };
📜 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 (8)
src/components/illustrations/IllustrationMeetingSchedules.tsx(1 hunks)src/components/illustrations/IllustrationMinistryAssignments.tsx(1 hunks)src/components/illustrations/IllustrationOtherSchedules.tsx(1 hunks)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(2 hunks)src/features/app_start/shared/illustration/useIllustration.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/components/illustrations/IllustrationMinistryAssignments.tsx
- src/components/illustrations/IllustrationOtherSchedules.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (9)
src/components/svg_as_image/index.types.ts (1)
1-6: Well-structured TypeScript type definition.The type
SVGAsImagePropsis properly defined as an extension of React'sDetailedHTMLPropsfor image elements, providing good type safety for the SVG-to-image component.src/components/svg_as_image/index.tsx (1)
4-11: Good documentation with JSDoc.The component is well-documented with JSDoc comments providing clear information about its purpose, parameters and return value.
src/features/app_start/shared/illustration/index.tsx (2)
26-26: Good use of CSS variables for theming.Replacing the hardcoded color with a CSS variable
var(--accent-main)improves theming flexibility.
71-71: Dynamic component rendering enhancement.Changing from a static image to rendering a component from
slide.componentallows for more flexible and dynamic content.src/components/illustrations/IllustrationMeetingSchedules.tsx (2)
1-4: Appropriate use of Material-UI SvgIcon.Using the Material-UI
SvgIconcomponent is a good approach for creating SVG illustrations that can easily integrate with the rest of the application.
13-341: Good use of CSS variables for theming SVG elements.The SVG uses CSS variables (like
var(--white-semi-s),var(--grey-200),var(--watchtower-study), etc.) for fill colors, which allows for theming and consistent color usage across the application.src/components/svg_as_image/useSVGAsImage.tsx (2)
1-5: The imports look goodThe necessary imports for state management, React hooks, and utility functions are correctly implemented.
11-33:❓ Verification inconclusive
Consider adding URL validation
The hook fetches SVGs from a URL without validating it, which could potentially lead to security issues if malicious URLs are provided.
🏁 Script executed:
#!/bin/bash # Check for other instances where URL validation is performed in the codebase rg -g "*.ts*" "validateURL|isValidUrl|URL validation" --no-filenameLength of output: 69
Enhance SVG URL Validation Prior to Fetch Operation
It appears that no URL validation helpers (e.g.,
validateURL/isValidUrl) are detected in the codebase. Since theconvertSvgToBase64function proceeds directly with the providedsvgUrl, please ensure that URLs are properly validated before usage to safeguard against potential security issues with untrusted input.
- File:
src/components/svg_as_image/useSVGAsImage.tsx(Lines 11-33)- Recommendation: Incorporate a URL validation function to verify that the
svgUrlis safe and correctly formatted prior to performing the fetch operation.src/features/app_start/shared/illustration/useIllustration.tsx (1)
5-11: Clean component imports organizationThe transition from static SVG imports to React components is well-structured and improves maintainability.
…G-illustrations_Max-Makaluk Signed-off-by: Max Makaluk <70341920+FussuChalice@users.noreply.github.com>
|
@ux-git, @FussuChalice, the following pictures are the ones that I mention that we can tweak:
Thanks. |
|
Yes, for the schedules must for sure be simply grey with no color change, and the ministry timer should have green colors when using the green theme. But one element is definitely missing a variable. |
|
@rhahao @ux-git That's it, I changed it, I think the cache needs to be cleared for the timer, everything works for me. |
organized-app
|
||||||||||||||||||||||||||||
| Project |
organized-app
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 00m 04s |
| Commit |
|
| Committer | Max Makaluk |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
1
|
| View all changes introduced in this branch ↗︎ | |
|
@FussuChalice I forgot to mention the schedules borders – they also must remain grey at all time, sorry 😅 |
Type of change
Thank you @rhahao for your help!
Checklist: