fix(icon): solve the style problem caused by multiple identical ids o… #2086
Conversation
WalkthroughThe pull request introduces enhancements to SVG handling in both Vue 2 and Vue 3 configurations within the Changes
Possibly related PRs
Tip New featuresWalkthrough comment now includes:
Notes:
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 (
|
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
packages/vue-common/src/generateIcon.ts (1)
36-43: Consider encapsulating theuniqueIdto avoid concurrency issues.The global
uniqueIdused ingenerateIconcould lead to issues in concurrent scenarios where multiple instances might interfere with each other. Consider encapsulating this state within a class or using a more robust method to generate unique IDs that are safe in concurrent environments.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- examples/sites/vite.config.ts (1 hunks)
- examples/vue2/vite.config.ts (1 hunks)
- examples/vue3/vite.config.ts (1 hunks)
- packages/modules.json (2 hunks)
- packages/vue-common/src/generateIcon.ts (1 hunks)
- packages/vue-common/src/index.ts (2 hunks)
Additional comments not posted (5)
examples/vue2/vite.config.ts (1)
32-46: Approve the enhanced SVG handling configuration.The updated configuration for
vue2SvgPluginis well-thought-out, addressing common issues with SVGs in web projects by ensuring theviewBoxis preserved and IDs are prefixed. This should effectively prevent conflicts and improve the handling of SVGs across the application.examples/vue3/vite.config.ts (1)
39-54: Approve the updated SVG plugin configuration.The configuration changes to
vue3SvgPluginare appropriate and align with best practices for handling SVGs in modern web applications. Setting the default import to 'component' and configuring SVG optimization plugins enhances the flexibility and usability of SVGs within the project.examples/sites/vite.config.ts (1)
49-64: Approved: Enhanced SVG handling in Vite configuration.The configuration changes for
vue3SvgPluginare correctly implemented to improve SVG handling:
- SVGs are now imported as components, which is a best practice in Vue 3.
- The
svgoConfigis properly set to retain theviewBoxand prefix IDs, addressing the PR's goal of solving style issues caused by multiple identical IDs.Consider verifying the rendering of SVGs in the application to ensure that there are no visual regressions or issues due to these configuration changes.
packages/vue-common/src/index.ts (1)
28-28: Approved: Enhancements to SVG icon handling in Vue components.The introduction of
GRADIENT_ICONS_LISTandgenerateIconis well-implemented to address the issue of duplicate SVG IDs:
- The logic to check and modify SVG IDs dynamically is a robust solution to prevent style conflicts.
- The changes ensure compatibility with Vue 2 by correctly binding and modifying the render function.
It's recommended to add unit tests to verify that SVG icons with gradients are rendered correctly without ID conflicts or other styling issues.
Also applies to: 286-299
packages/modules.json (1)
87-91: Approved: New module definitions forAnchorMobileFirstandHooks.The additions of
AnchorMobileFirstandHooksmodules are correctly configured:
AnchorMobileFirstsupports the mobile-first design strategy.Hooksmodule likely provides reusable state management functions, enhancing modularity.Ensure that documentation is updated to include information on the roles and usage of these new modules, helping developers understand their integration and purpose in the project.
Also applies to: 1473-1477
| const generateId = (vnode, idMaps) => { | ||
| if (vnode.props?.id) { | ||
| const newId = `${vnode.props.id}${uniqueId}` | ||
| idMaps[vnode.props.id] = newId | ||
| vnode.props.id = newId | ||
| } | ||
|
|
||
| if (vnode?.children) { | ||
| vnode?.children.forEach((item) => { | ||
| generateId(item, idMaps) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Document the side effects of modifying input objects.
The generateId function modifies the vnode object and its children directly by changing their id properties. It's important to document these side effects clearly in the function's documentation to avoid unexpected behavior for users of this function.
Would you like me to help draft the documentation for this function?
| const generateUrl = (vnode, idMaps) => { | ||
| const checkList = ['fill', 'mask'] | ||
|
|
||
| checkList.forEach((item) => { | ||
| if (vnode.props?.[item]?.includes('url(#')) { | ||
| const oldId = vnode.props[item].replace('url(#', '').replace(')', '') | ||
| const newId = idMaps[oldId] | ||
| vnode.props[item] = `url(#${newId})` | ||
| } | ||
| }) | ||
|
|
||
| if (vnode.children) { | ||
| vnode.children.forEach((item) => { | ||
| generateUrl(item, idMaps) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Enhance robustness in URL handling.
The generateUrl function assumes a specific format when extracting and replacing IDs in URLs. Consider using a more robust method to handle different possible URL formats, such as using regular expressions to match and replace the ID part of the URL.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/vue-common/src/generateIcon.ts (1 hunks)
- packages/vue-common/src/index.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/vue-common/src/generateIcon.ts
- packages/vue-common/src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/vue-common/src/generateIcon.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/vue-common/src/generateIcon.ts
…f gradient svg
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Bug Fixes