Chore/svg attribute fix#7446
Conversation
WalkthroughThis update modifies several React SVG icon components to use camelCase attribute names for SVG properties, aligning with React's JSX conventions. Additionally, the Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
♻️ Duplicate comments (1)
packages/ui/src/icons/pending-icon.tsx (1)
21-22: Same attribute fix as above
Mirrors the correction in the first path element; no further issues.
🧹 Nitpick comments (7)
packages/ui/src/icons/at-risk-icon.tsx (1)
1-1: Consider dropping the explicit React import if the repo already uses the new JSX transformSince React 17 the
jsx/tsxtransform no longer requiresimport React from "react"at the top of every file (unless you rely on the classic runtime or useReactdirectly). If the project is on React 17+ with"jsx": "react-jsx"(or"react-jsxdev") intsconfig.json, you can safely remove this line to eliminate an unused import.packages/ui/src/icons/done-icon.tsx (1)
14-20: Expose colour via the existingcolorprop for consistency.
coloris accepted in the component signature but never used. Hard-coding#15A34Ahere forces a single colour and weakens theming. Consider:- <circle cx="5" cy="5.5" r="4.4" stroke="#15A34A" strokeWidth="1.2" /> + <circle cx="5" cy="5.5" r="4.4" stroke={color ?? "#15A34A"} strokeWidth="1.2" /> … - fill="#15A34A" + fill={color ?? "#15A34A"}This keeps the current appearance by default while letting callers override it when needed.
apps/live/package.json (2)
10-13: Rethink switching from Node--env-filetodotenv-cli.Node ≥ 20 already supports
--env-file, which means the previous implementation worked without an extra runtime dependency.
By replacing it withdotenv-cliwe:
- Add a new prod dependency (≈ 140 KB installed) that brings its own transitive tree and potential CVE surface.
- Lose the guarantee that we are using the same
.envparsing semantics asdotenvin the application code (e.g. multiline / escaped values).- Introduce slightly slower start-up (extra process spawn).
Unless you need features missing from the built-in flag — such as multiple
-efiles or--examplevalidation — you can keep the original, simpler script:- "dev": "tsup --watch --onSuccess \"dotenv -e .env -- node dist/server.js\"", + "dev": "tsup --watch --onSuccess \"node --env-file=.env dist/server.js\"", … - "start": "dotenv -e .env -- node dist/server.js", + "start": "node --env-file=.env dist/server.js",If you decide to keep
dotenv-cli, please add a short comment in the README explaining the specific requirement so future maintainers do not revert it inadvertently.
46-46:dotenv-clishould probably live indevDependencies.The package is only needed at launch time for the host process, not inside the bundled code produced by
tsup.
Moving it keeps the production footprint minimal:- "dotenv-cli": "^8.0.0",and in the
devDependenciesblock:+ "dotenv-cli": "^8.0.0",Double-check the deployment workflow: if production containers invoke
npm startdirectly without runningnpm install --production, leaving it independenciesis acceptable.packages/ui/src/icons/sticky-note-icon.tsx (1)
5-14: Optional: expose an accessible name / role for screen-reader users.These decorative icons currently have no accessibility attributes. Adding either
aria-hiddenfor purely decorative use orrole="img" aria-label="Sticky note"(with a prop override) will improve a11y compliance.<svg width={width} height={height} className={className} viewBox="0 0 17 17" fill={"currentColor"} xmlns="http://www.w3.org/2000/svg" style={{ color }} + aria-hidden={typeof className === "undefined" ? true : undefined} + role={typeof className !== "undefined" ? "img" : undefined} + aria-label={typeof className !== "undefined" ? "Sticky note" : undefined} >packages/ui/src/icons/multiple-sticky.tsx (1)
5-14: Optional: expose intent with accessibility attributesAll icons in the library appear to be decorative. Explicitly declaring this helps assistive tech and avoids the SVG being exposed in the a11y tree.
{...rest} + aria-hidden="true" + focusable="false" + role="img"(If any icon is meant to be announced, provide a
<title>child instead.)packages/ui/src/icons/planned-icon.tsx (1)
5-13:colorprop is accepted but never applied
PlannedStatedestructurescoloryet no element uses it, so the consumer cannot actually set the icon color. Either:
- Pass
colorto allfillattributes, or- Remove the unused prop to avoid a misleading API.
Example of option 1:
- fill="#455068" + fill={color ?? "#455068"}(then do the same for the other two
<path>elements).This keeps the component predictable and avoids dead props.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
apps/live/package.json(2 hunks)packages/ui/src/icons/at-risk-icon.tsx(1 hunks)packages/ui/src/icons/done-icon.tsx(1 hunks)packages/ui/src/icons/lead-icon.tsx(1 hunks)packages/ui/src/icons/multiple-sticky.tsx(1 hunks)packages/ui/src/icons/off-track-icon.tsx(1 hunks)packages/ui/src/icons/on-track-icon.tsx(1 hunks)packages/ui/src/icons/pending-icon.tsx(1 hunks)packages/ui/src/icons/planned-icon.tsx(1 hunks)packages/ui/src/icons/sticky-note-icon.tsx(1 hunks)packages/ui/src/icons/updates-icon.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
packages/ui/src/icons/multiple-sticky.tsx (1)
Learnt from: lifeiscontent
PR: makeplane/plane#7164
File: packages/ui/.storybook/main.ts:24-47
Timestamp: 2025-06-04T16:22:44.344Z
Learning: In packages/ui/.storybook/main.ts, the webpackFinal function intentionally overrides the CSS loader strategy by finding and replacing existing CSS rules. This is a temporary workaround for a known upstream issue in Storybook's CSS handling that has been communicated to the Storybook maintainers. The current implementation should not be changed until the upstream issue is resolved.
packages/ui/src/icons/done-icon.tsx (1)
Learnt from: lifeiscontent
PR: makeplane/plane#7164
File: packages/ui/.storybook/main.ts:24-47
Timestamp: 2025-06-04T16:22:44.344Z
Learning: In packages/ui/.storybook/main.ts, the webpackFinal function intentionally overrides the CSS loader strategy by finding and replacing existing CSS rules. This is a temporary workaround for a known upstream issue in Storybook's CSS handling that has been communicated to the Storybook maintainers. The current implementation should not be changed until the upstream issue is resolved.
🔇 Additional comments (11)
packages/ui/src/icons/pending-icon.tsx (1)
15-16: Camel-cased SVG attributes resolve React DOM warnings – LGTM
The switch tofillRule/clipRuleis correct for JSX and eliminates the invalid-prop console noise.packages/ui/src/icons/lead-icon.tsx (2)
11-11: LGTM: Correct JSX attribute conversionThe
clipPathattribute correctly replaces the kebab-caseclip-pathto align with React's JSX conventions.
14-15: LGTM: Proper camelCase attribute conversionThe conversion from
fill-ruletofillRuleandclip-ruletoclipRulecorrectly addresses JSX compliance requirements. This eliminates console warnings and ensures proper SVG rendering in React components.packages/ui/src/icons/on-track-icon.tsx (1)
7-28: LGTM! Correct JSX attribute naming conversion.The changes properly convert SVG attributes from kebab-case to camelCase for React JSX compatibility:
clip-path→clipPathfill-rule→fillRuleclip-rule→clipRuleThis fixes the console warnings about invalid DOM properties while maintaining the same visual output and functionality.
packages/ui/src/icons/at-risk-icon.tsx (1)
7-10: SVG attribute casing fixed – resolves React warningsChanging
clipPath,fillRule, andclipRuleto camel-case is the correct approach for JSX and will silence the “invalid DOM property” console warnings. No functional or visual regressions expected.packages/ui/src/icons/off-track-icon.tsx (1)
9-10: Camel-case conversion offillRule/clipRuleis correct.Nice catch—this resolves the React console warnings for these attributes.
packages/ui/src/icons/done-icon.tsx (2)
14-14: Attribute casing fixed — looks good.Converting
stroke-width→strokeWidthremoves the React warning and preserves SVG behaviour.
16-17:fillRule/clipRulecamel-cased correctly.Same JSX-compliance fix as above; no further issues spotted.
packages/ui/src/icons/sticky-note-icon.tsx (1)
26-27: Camel-case fix aligns with JSX conventions – looks good.
fillRule/clipRuleare the correct React-friendly attribute names and eliminate the previous console warnings.packages/ui/src/icons/multiple-sticky.tsx (1)
16-17: Attributes correctly converted to camelCase – React warning resolvedThe change from
fill-rule/clip-ruletofillRule/clipRulematches JSX conventions and prevents the “Invalid DOM property” console noise. Nice tidy-up.Also applies to: 22-23
packages/ui/src/icons/updates-icon.tsx (1)
8-9: Correct JSX camelCase attributes – looks good
fillRuleandclipRulereplace the kebab-case variants, removing React’s invalid-prop warnings while preserving SVG semantics.
sriramveeraghanta
merged commit 2dc1c25
into
makeplane:fix-svg-attributes
* chore: svg attribute fix (#7446) * fix: replace invalid --env-file with dotenv-cli for tsup onSuccess * fix: replace invalid --env-file usage in start script and move dotenv-cli to dependencies * chor: update SVG attributes to camelCase for JSX compatibility * chor: update SVG attributes to camelCase * fix: removed dontenv-cli * fix: svg attibute fixes * chore: minor attribute customizations --------- Co-authored-by: Donal Noye <68372408+Donal-Noye@users.noreply.github.com>
* chore: svg attribute fix (#7446) * fix: replace invalid --env-file with dotenv-cli for tsup onSuccess * fix: replace invalid --env-file usage in start script and move dotenv-cli to dependencies * chor: update SVG attributes to camelCase for JSX compatibility * chor: update SVG attributes to camelCase * fix: removed dontenv-cli * fix: svg attibute fixes * chore: minor attribute customizations --------- Co-authored-by: Donal Noye <68372408+Donal-Noye@users.noreply.github.com>
2 participants
Description
This PR updates multiple SVG icon files by replacing non-JSX-compliant attributes fill-rule and clip-rule with their camelCase equivalents fillRule and clipRule.
This ensures proper rendering of SVGs in React and prevents console warnings
Type of Change
Screenshots and Media (if applicable)
N/A — purely code-level SVG attribute fixes
Test Scenarios
References
None
Summary by CodeRabbit
Chores
Style