Shade updates #22045
Shade updates #22045
Conversation
WalkthroughThis pull request involves significant refactoring and documentation improvements in the Shade design system. The changes include removing the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)
🪧 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: 4
🧹 Nitpick comments (17)
apps/shade/src/docs/Welcome.mdx (1)
2-2: Consider adding image dimensions for better performance.To prevent Cumulative Layout Shift (CLS) and improve Core Web Vitals, consider adding width and height attributes to the image.
- <img src={techStackImage} alt="Technology Stack" /> + <img src={techStackImage} alt="Technology Stack" width={/* width */} height={/* height */} />Also applies to: 14-14
apps/shade/src/docs/Conventions.mdx (2)
29-30: Enhance clarity and fix grammar in the TailwindCSS color naming section.Consider revising the text for better clarity and grammar:
-For gray colors up until now we've used `grey`. ShadCN follows TailwindCSS naming conventions and uses `gray` when you install a component. We also decided to go with this mainly to avoid having to manually override color names and risking manual errors in the design system. +Previously, we used `grey` for gray colors. However, since ShadCN follows TailwindCSS naming conventions and uses `gray`, we've decided to adopt this convention to avoid manual overrides and potential errors in the design system.🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: A comma might be missing here.
Context: ... ## TailwindCSS color naming For gray colors up until now we've usedgrey. ShadCN ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[grammar] ~29-~29: Did you mean “until” or “up to”?
Context: ...ilwindCSS color naming For gray colors up until now we've usedgrey. ShadCN follows T...(UP_UNTIL)
[uncategorized] ~29-~29: Possible missing comma found.
Context: ...a component. We also decided to go with this mainly to avoid having to manually over...(AI_HYDRA_LEO_MISSING_COMMA)
33-40: Improve professionalism and clarity in the filename conventions section.The content needs several improvements:
- Fix subject-verb agreement
- Use proper capitalization for macOS
- Replace informal language with professional terminology
- Add missing punctuation
-Our generic naming conventions is: +Our generic naming conventions are: -When you install a ShadCN component via the CLI it'll create files with kebab-case. This is _not_ following our standards. Unfortunately changing _only_ filename casing in MacOS and Github is a **massive** PITA so for now we're accepting this inconsistency and let ShadCN create its files as is. +When you install a ShadCN component via the CLI, it creates files with kebab-case. This does _not_ follow our standards. However, changing _only_ filename casing in macOS and GitHub presents significant technical challenges, so for now, we accept this inconsistency and allow ShadCN to create its files as is.🧰 Tools
🪛 LanguageTool
[grammar] ~33-~33: The verb form ‘is’ does not seem to match the subject ‘conventions’.
Context: ...lenames Our generic naming conventions is: -PascalCasefor React component na...(SUBJECT_VERB_AGREEMENT_PLURAL)
[uncategorized] ~39-~39: A comma might be missing here.
Context: ... you install a ShadCN component via the CLI it'll create files with kebab-case. Thi...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~39-~39: A comma might be missing here.
Context: ... This is not following our standards. Unfortunately changing only filename casing in MacO...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[grammar] ~39-~39: The operating system from Apple is written “macOS”.
Context: ...tely changing only filename casing in MacOS and Github is a massive PITA so for...(MAC_OS)
[typographical] ~39-~39: Consider adding a comma here.
Context: ...cOS and Github is a massive PITA so for now we're accepting this inconsistency and ...(FOR_NOW_COMMA)
apps/shade/src/lib/utils.ts (3)
11-34: Consider enhancing type safety.The debounce implementation is solid but could be improved with:
- Return type for the debounced function
- Explicit void return type for the later function
-export function debounce<T extends unknown[]>(func: (...args: T) => void, wait: number, immediate: boolean = false): (...args: T) => void { +export function debounce<T extends unknown[], R>( + func: (...args: T) => R, + wait: number, + immediate: boolean = false +): (...args: T) => void { let timeoutId: ReturnType<typeof setTimeout> | null; return function (this: unknown, ...args: T): void { - const later = () => { + const later = (): void => { timeoutId = null; if (!immediate) { func.apply(this, args); } };
37-41: Consider handling edge cases.The function works well for standard cases but could be more robust by:
- Handling empty strings
- Handling consecutive hyphens/underscores
export const kebabToPascalCase = (str: string): string => { + if (!str) return ''; + // Remove consecutive hyphens/underscores + const normalized = str.replace(/[-_]+/g, '-'); const processed = str .replace(/[-_]([a-z0-9])/gi, (_, char) => char.toUpperCase()); return processed.charAt(0).toUpperCase() + processed.slice(1); };
44-127: Consider breaking down complex URL formatting logic.While the implementation is thorough, it could benefit from:
- Breaking down into smaller, focused functions
- Additional error handling for malformed URLs
- Handling of data URLs and other special protocols
Consider extracting the protocol handling logic:
const handleSpecialProtocols = (url: string) => { if (isEmail(url)) { return {save: `mailto:${url}`, display: `mailto:${url}`}; } if (url.match(/^(data:|blob:|javascript:)/i)) { return {save: url, display: url}; } return null; };apps/shade/src/docs/CreatingComponents.mdx (3)
17-17: Add hyphenation for compound adjectives.Update "third party" to "third-party" when used as a compound adjective.
-⚠️ Sometimes the npx command fails with a massive error. This usually happens if ShadCN tries to reinstall an already existing third party library +⚠️ Sometimes the npx command fails with a massive error. This usually happens if ShadCN tries to reinstall an already existing third-party library🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... tries to reinstall an already existing third party library or component (e.g. `radix-ui/di...(EN_COMPOUND_ADJECTIVE_INTERNAL)
110-111: Improve clarity and grammar in third-party library note.The note about third-party libraries could be clearer and more grammatically correct.
-**Note:** ShadCN uses a few third party libraries. Since we export everything from Shade under the project `@tryghost/shade` there might be conflicts if a third party library uses similar component names as some other components in Shade. For example, ShadCN uses Recharts to display charts. Recharts has a `<Tooltip>` component and Shade also has one. To overcome this issue we alias all third party exports (e.g. `export * as Recharts from "recharts"`). +**Note:** ShadCN uses a few third-party libraries. Since we export everything from Shade under the project `@tryghost/shade`, there might be conflicts if a third-party library uses similar component names as other components in Shade. For example, ShadCN uses Recharts to display charts. Recharts has a `<Tooltip>` component, and Shade also has one. To overcome this issue, we alias all third-party exports (e.g., `export * as Recharts from "recharts"`).🧰 Tools
🪛 LanguageTool
[uncategorized] ~110-~110: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...apps. --- Note: ShadCN uses a few third party libraries. Since we export everything f...(EN_COMPOUND_ADJECTIVE_INTERNAL)
[typographical] ~110-~110: Consider adding a comma.
Context: ...hade under the project@tryghost/shadethere might be conflicts if a third party lib...(IF_THERE_COMMA)
[uncategorized] ~110-~110: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...st/shade` there might be conflicts if a third party library uses similar component names as...(EN_COMPOUND_ADJECTIVE_INTERNAL)
[typographical] ~110-~110: It seems that a comma is missing.
Context: ...nd Shade also has one. To overcome this issue we alias all third party exports (e.g. ...(IN_ORDER_TO_VB_COMMA)
[uncategorized] ~110-~110: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ne. To overcome this issue we alias all third party exports (e.g. `export * as Recharts fro...(EN_COMPOUND_ADJECTIVE_INTERNAL)
136-138: Improve clarity in component creation guidelines.The guidelines for creating custom components could be more specific.
-Create a single file for each UI component -Create [composable components](https://blog.tomaszgil.me/choosing-the-right-path-composable-vs-configurable-components-in-react) (multiple React components), _not_ configurable ones (lots of props). Create all corresponding React components in the same file (take a look at the Dropdown Menu implementation for an example). +- Create a single file for each UI component +- Create [composable components](https://blog.tomaszgil.me/choosing-the-right-path-composable-vs-configurable-components-in-react) (multiple React components) instead of configurable ones with numerous props +- Keep related React components in the same file (refer to the Dropdown Menu implementation as an example)🧰 Tools
🪛 LanguageTool
[style] ~137-~137: The phrase ‘lots of’ might be wordy and overused. Consider using an alternative.
Context: ...t components), not configurable ones (lots of props). Create all corresponding React ...(A_LOT_OF)
apps/shade/src/components/ui/separator.stories.tsx (1)
13-15: Consider adding more stories to showcase component variants.The default story with empty args doesn't fully demonstrate the component's capabilities. Consider adding stories for different orientations (horizontal/vertical) and styling variants.
apps/shade/src/components/ui/badge.stories.tsx (1)
13-17: Enhance documentation with additional badge variants.Consider adding stories that demonstrate:
- Different badge variants (success, warning, error)
- Size variations
- With/without icons
apps/shade/src/components/ui/avatar.stories.tsx (1)
20-35: Add stories for error states and loading states.The current stories show successful cases. Consider adding:
- Story demonstrating image load failure
- Story showing loading state
- Story with different avatar sizes
apps/shade/src/components/ui/tooltip.stories.tsx (1)
28-37: Enhance documentation with positioning and accessibility examples.Consider adding:
- Stories demonstrating different tooltip positions (top, right, bottom, left)
- Custom delay examples
- Documentation about keyboard accessibility
- Examples of rich tooltip content
apps/shade/src/components/ui/dialog.stories.tsx (1)
21-34: Enhance dialog story with more variants and accessibility features.While the basic implementation is good, consider enhancing it with:
- Additional variants showing different use cases (confirmation, form, etc.)
- Accessibility attributes like
aria-describedbyfor better screen reader support- Examples of dialog with different content layouts
export const Default: Story = { args: { children: ( <> - <DialogTrigger className='cursor-pointer'><Button className='cursor-pointer'>Open</Button></DialogTrigger> + <DialogTrigger className='cursor-pointer'> + <Button aria-label="Open dialog" className='cursor-pointer'>Open</Button> + </DialogTrigger> <DialogContent> <DialogHeader> <DialogTitle>Are you absolutely sure?</DialogTitle> + <p id="dialog-description">This action cannot be undone.</p> </DialogHeader> </DialogContent> </> ) } }; + +export const WithForm: Story = { + args: { + children: ( + // Add a form dialog example + ) + } +};apps/shade/src/components/ui/table.stories.tsx (1)
20-50: Enhance table story with more real-world examples.The current example is good but could be enhanced with:
- Responsive design patterns for mobile views
- Interactive features like sorting and pagination
- Additional variants showing different use cases (e.g., data grids, expandable rows)
export const Default: Story = { args: { children: ( <> - <TableCaption>A list of your recent invoices.</TableCaption> + <TableCaption className="mb-4">A list of your recent invoices.</TableCaption> <TableHeader> <TableRow> - <TableHead className="w-[100px]">Invoice</TableHead> + <TableHead className="w-[100px] md:w-[150px]">Invoice</TableHead> <TableHead>Status</TableHead> <TableHead>Method</TableHead> - <TableHead className="text-right">Amount</TableHead> + <TableHead className="text-right hidden md:table-cell">Amount</TableHead> </TableRow> </TableHeader>Consider adding more stories:
export const WithPagination: Story = { // Add example with pagination }; export const WithSorting: Story = { // Add example with sortable columns };apps/shade/src/components/ui/chart.stories.tsx (2)
23-24: Avoid hard-coding CSS variable references.The chart data uses CSS variables that might not be defined in all contexts.
- {browser: 'chrome', visitors: 98, fill: 'var(--color-chrome)'}, - {browser: 'safari', visitors: 17, fill: 'var(--color-safari)'} + {browser: 'chrome', visitors: 98, fill: chartConfig.chrome.color}, + {browser: 'safari', visitors: 17, fill: chartConfig.safari.color}
96-98: Consider internalizing documentation links.External documentation links might break or become outdated. Consider maintaining internal documentation or versioning the links.
- Visit <a className="underline" href="https://ui.shadcn.com/docs/components/chart" rel="noreferrer" target="_blank">ShadCN/UI Charts docs</a> for usage details. + Visit <a className="underline" href="/docs/components/chart">Charts documentation</a> for usage details.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/shade/src/docs/assets/tech-stack.pngis excluded by!**/*.png
📒 Files selected for processing (22)
apps/shade/src/boilerplate.stories.tsx(0 hunks)apps/shade/src/boilerplate.tsx(0 hunks)apps/shade/src/components/ui/avatar.stories.tsx(1 hunks)apps/shade/src/components/ui/badge.stories.tsx(1 hunks)apps/shade/src/components/ui/chart.stories.tsx(1 hunks)apps/shade/src/components/ui/dialog.stories.tsx(1 hunks)apps/shade/src/components/ui/icon.ts(1 hunks)apps/shade/src/components/ui/separator.stories.tsx(1 hunks)apps/shade/src/components/ui/table.stories.tsx(1 hunks)apps/shade/src/components/ui/tooltip.stories.tsx(1 hunks)apps/shade/src/components/ui/tooltip.tsx(1 hunks)apps/shade/src/docs/Conventions.mdx(1 hunks)apps/shade/src/docs/CreatingComponents.mdx(6 hunks)apps/shade/src/docs/Environment.mdx(2 hunks)apps/shade/src/docs/UsingComponents.mdx(1 hunks)apps/shade/src/docs/Welcome.mdx(2 hunks)apps/shade/src/index.ts(1 hunks)apps/shade/src/lib/utils.ts(1 hunks)apps/shade/src/providers/ShadeProvider.tsx(1 hunks)apps/shade/src/utils/debounce.ts(0 hunks)apps/shade/src/utils/formatText.ts(0 hunks)apps/shade/src/utils/formatUrl.ts(0 hunks)
💤 Files with no reviewable changes (5)
- apps/shade/src/boilerplate.tsx
- apps/shade/src/boilerplate.stories.tsx
- apps/shade/src/utils/formatText.ts
- apps/shade/src/utils/formatUrl.ts
- apps/shade/src/utils/debounce.ts
✅ Files skipped from review due to trivial changes (5)
- apps/shade/src/docs/UsingComponents.mdx
- apps/shade/src/docs/Environment.mdx
- apps/shade/src/components/ui/icon.ts
- apps/shade/src/components/ui/tooltip.tsx
- apps/shade/src/providers/ShadeProvider.tsx
🧰 Additional context used
🪛 LanguageTool
apps/shade/src/docs/CreatingComponents.mdx
[uncategorized] ~17-~17: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... tries to reinstall an already existing third party library or component (e.g. `radix-ui/di...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~110-~110: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...apps. --- Note: ShadCN uses a few third party libraries. Since we export everything f...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[typographical] ~110-~110: Consider adding a comma.
Context: ...hade under the project @tryghost/shade there might be conflicts if a third party lib...
(IF_THERE_COMMA)
[uncategorized] ~110-~110: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...st/shade` there might be conflicts if a third party library uses similar component names as...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[typographical] ~110-~110: It seems that a comma is missing.
Context: ...nd Shade also has one. To overcome this issue we alias all third party exports (e.g. ...
(IN_ORDER_TO_VB_COMMA)
[uncategorized] ~110-~110: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ne. To overcome this issue we alias all third party exports (e.g. `export * as Recharts fro...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~137-~137: The phrase ‘lots of’ might be wordy and overused. Consider using an alternative.
Context: ...t components), not configurable ones (lots of props). Create all corresponding React ...
(A_LOT_OF)
apps/shade/src/docs/Conventions.mdx
[uncategorized] ~29-~29: A comma might be missing here.
Context: ... ## TailwindCSS color naming For gray colors up until now we've used grey. ShadCN ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[grammar] ~29-~29: Did you mean “until” or “up to”?
Context: ...ilwindCSS color naming For gray colors up until now we've used grey. ShadCN follows T...
(UP_UNTIL)
[uncategorized] ~29-~29: Possible missing comma found.
Context: ...a component. We also decided to go with this mainly to avoid having to manually over...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~33-~33: The verb form ‘is’ does not seem to match the subject ‘conventions’.
Context: ...lenames Our generic naming conventions is: - PascalCase for React component na...
(SUBJECT_VERB_AGREEMENT_PLURAL)
[uncategorized] ~39-~39: A comma might be missing here.
Context: ... you install a ShadCN component via the CLI it'll create files with kebab-case. Thi...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~39-~39: A comma might be missing here.
Context: ... This is not following our standards. Unfortunately changing only filename casing in MacO...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[grammar] ~39-~39: The operating system from Apple is written “macOS”.
Context: ...tely changing only filename casing in MacOS and Github is a massive PITA so for...
(MAC_OS)
[typographical] ~39-~39: Consider adding a comma here.
Context: ...cOS and Github is a massive PITA so for now we're accepting this inconsistency and ...
(FOR_NOW_COMMA)
[uncategorized] ~45-~45: Possible missing comma found.
Context: ...le components (such as a page or header container etc.) - /src/hooks/ — Custom Reach ho...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Browser tests
🔇 Additional comments (4)
apps/shade/src/docs/Welcome.mdx (1)
16-16: LGTM! Clear documentation of requirements.The added requirement for Storybook entries aligns well with the PR objectives and establishes a clear standard for component documentation and testing.
apps/shade/src/lib/utils.ts (1)
1-8: LGTM! Clean utility setup.The imports and
cnutility function follow best practices for combining Tailwind classes.apps/shade/src/components/ui/avatar.stories.tsx (1)
30-30: Consider using a more stable image source.The GitHub avatar URL might change or become inaccessible. Consider:
- Using a local image asset
- Using a dedicated image hosting service
- Adding error handling documentation
apps/shade/src/components/ui/tooltip.stories.tsx (1)
9-15: LGTM! Good use of TooltipProvider as a decorator.The decorator ensures proper tooltip context for all stories.
| - `/src/components/` — Root directory for components | ||
| - `/src/components/ui/` — Directory for atomic UI components (foundational UI elements such as buttons, dropdowns etc.) | ||
| - `/src/components/layout` — Complex, globally reusable components (such as a page or header container etc.) | ||
| - `/src/hooks/` — Custom Reach hooks | ||
| - `/src/providers/` — Context providers | ||
| - `/src/ib/utils.ts` — Utilities | ||
| - `/src/docs/` — Shade documentation |
There was a problem hiding this comment.
Fix typos and improve consistency in the file structure section.
There are a few issues to address:
- "Reach hooks" should be "React hooks"
- The utils path appears incorrect ("/src/ib/utils.ts")
- Inconsistent use of punctuation in directory descriptions
- `/src/components/` — Root directory for components
- `/src/components/ui/` — Directory for atomic UI components (foundational UI elements such as buttons, dropdowns etc.)
- `/src/components/layout` — Complex, globally reusable components (such as a page or header container etc.)
-- `/src/hooks/` — Custom Reach hooks
+- `/src/hooks/` — Custom React hooks
- `/src/providers/` — Context providers
-- `/src/ib/utils.ts` — Utilities
+- `/src/lib/utils.ts` — Utilities
- `/src/docs/` — Shade documentation📝 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.
| - `/src/components/` — Root directory for components | |
| - `/src/components/ui/` — Directory for atomic UI components (foundational UI elements such as buttons, dropdowns etc.) | |
| - `/src/components/layout` — Complex, globally reusable components (such as a page or header container etc.) | |
| - `/src/hooks/` — Custom Reach hooks | |
| - `/src/providers/` — Context providers | |
| - `/src/ib/utils.ts` — Utilities | |
| - `/src/docs/` — Shade documentation | |
| - `/src/components/` — Root directory for components | |
| - `/src/components/ui/` — Directory for atomic UI components (foundational UI elements such as buttons, dropdowns etc.) | |
| - `/src/components/layout` — Complex, globally reusable components (such as a page or header container etc.) | |
| - `/src/hooks/` — Custom React hooks | |
| - `/src/providers/` — Context providers | |
| - `/src/lib/utils.ts` — Utilities | |
| - `/src/docs/` — Shade documentation |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~45-~45: Possible missing comma found.
Context: ...le components (such as a page or header container etc.) - /src/hooks/ — Custom Reach ho...
(AI_HYDRA_LEO_MISSING_COMMA)
| const displayFromBase = (url: string, baseUrl: string) => { | ||
| // Ensure base url has a trailing slash | ||
| if (!baseUrl.endsWith('/')) { | ||
| baseUrl += '/'; | ||
| } | ||
|
|
||
| // Remove leading slash from url | ||
| if (url.startsWith('/')) { | ||
| url = url.substring(1); | ||
| } | ||
|
|
||
| return new URL(url, baseUrl).toString(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for URL construction.
The function should handle URL construction errors gracefully.
const displayFromBase = (url: string, baseUrl: string) => {
// Ensure base url has a trailing slash
if (!baseUrl.endsWith('/')) {
baseUrl += '/';
}
// Remove leading slash from url
if (url.startsWith('/')) {
url = url.substring(1);
}
- return new URL(url, baseUrl).toString();
+ try {
+ return new URL(url, baseUrl).toString();
+ } catch (e) {
+ return url;
+ }
};📝 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 displayFromBase = (url: string, baseUrl: string) => { | |
| // Ensure base url has a trailing slash | |
| if (!baseUrl.endsWith('/')) { | |
| baseUrl += '/'; | |
| } | |
| // Remove leading slash from url | |
| if (url.startsWith('/')) { | |
| url = url.substring(1); | |
| } | |
| return new URL(url, baseUrl).toString(); | |
| }; | |
| const displayFromBase = (url: string, baseUrl: string) => { | |
| // Ensure base url has a trailing slash | |
| if (!baseUrl.endsWith('/')) { | |
| baseUrl += '/'; | |
| } | |
| // Remove leading slash from url | |
| if (url.startsWith('/')) { | |
| url = url.substring(1); | |
| } | |
| try { | |
| return new URL(url, baseUrl).toString(); | |
| } catch (e) { | |
| return url; | |
| } | |
| }; |
| export {default as useGlobalDirtyState} from './hooks/use-global-dirty-state'; | ||
|
|
||
| // Utils | ||
| export * from '@/lib/utils'; | ||
| export {cn} from '@/lib/utils'; | ||
| export {debounce} from './utils/debounce'; | ||
| export {formatUrl} from './utils/formatUrl'; | ||
| export {cn, debounce, kebabToPascalCase, formatUrl} from '@/lib/utils'; |
There was a problem hiding this comment.
Remove duplicate exports and clarify Tooltip exports.
There are two issues in this section:
- Duplicate exports from
@/lib/utils(line 41 exports everything, then line 42 exports specific functions) - Tooltip component exports are commented out but still included via wildcard export
export {default as useGlobalDirtyState} from './hooks/use-global-dirty-state';
// Utils
export * from '@/lib/utils';
-export {cn, debounce, kebabToPascalCase, formatUrl} from '@/lib/utils';Also, please decide on a single approach for Tooltip exports:
-// export {Tooltip as ShadeTooltip, TooltipTrigger, TooltipContent, TooltipProvider} from './components/ui/tooltip';
-export * from './components/ui/tooltip';
+export {Tooltip as ShadeTooltip, TooltipTrigger, TooltipContent, TooltipProvider} from './components/ui/tooltip';Committable suggestion skipped: line range outside the PR's diff.
| if (viewBox && 'cx' in viewBox && 'cy' in viewBox) { | ||
| return ( | ||
| <text | ||
| dominantBaseline="middle" | ||
| textAnchor="middle" | ||
| x={viewBox.cx} | ||
| y={viewBox.cy} | ||
| > | ||
| <tspan | ||
| className="fill-foreground text-2xl font-semibold tracking-tight" | ||
| x={viewBox.cx} | ||
| y={viewBox.cy} | ||
| > | ||
| {totalVisitors.toLocaleString()} | ||
| </tspan> | ||
| <tspan | ||
| className="fill-muted-foreground" | ||
| x={viewBox.cx} | ||
| y={(viewBox.cy || 0) + 20} | ||
| > | ||
| Reactions | ||
| </tspan> | ||
| </text> | ||
| ); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and error handling in chart label rendering.
The current implementation has several areas for improvement:
- Type assertion for viewBox properties could be safer
- Missing error handling for edge cases
- Hard-coded CSS variables could cause styling issues
- if (viewBox && 'cx' in viewBox && 'cy' in viewBox) {
+ const {cx = 0, cy = 0} = viewBox ?? {};
+ if (typeof cx !== 'number' || typeof cy !== 'number') {
+ console.warn('Invalid viewBox properties');
+ return null;
+ }
return (
<text
dominantBaseline="middle"
textAnchor="middle"
- x={viewBox.cx}
- y={viewBox.cy}
+ x={cx}
+ y={cy}
>📝 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.
| if (viewBox && 'cx' in viewBox && 'cy' in viewBox) { | |
| return ( | |
| <text | |
| dominantBaseline="middle" | |
| textAnchor="middle" | |
| x={viewBox.cx} | |
| y={viewBox.cy} | |
| > | |
| <tspan | |
| className="fill-foreground text-2xl font-semibold tracking-tight" | |
| x={viewBox.cx} | |
| y={viewBox.cy} | |
| > | |
| {totalVisitors.toLocaleString()} | |
| </tspan> | |
| <tspan | |
| className="fill-muted-foreground" | |
| x={viewBox.cx} | |
| y={(viewBox.cy || 0) + 20} | |
| > | |
| Reactions | |
| </tspan> | |
| </text> | |
| ); | |
| } | |
| }} | |
| const {cx = 0, cy = 0} = viewBox ?? {}; | |
| if (typeof cx !== 'number' || typeof cy !== 'number') { | |
| console.warn('Invalid viewBox properties'); | |
| return null; | |
| } | |
| return ( | |
| <text | |
| dominantBaseline="middle" | |
| textAnchor="middle" | |
| x={cx} | |
| y={cy} | |
| > | |
| <tspan | |
| className="fill-foreground text-2xl font-semibold tracking-tight" | |
| x={viewBox.cx} | |
| y={viewBox.cy} | |
| > | |
| {totalVisitors.toLocaleString()} | |
| </tspan> | |
| <tspan | |
| className="fill-muted-foreground" | |
| x={viewBox.cx} | |
| y={(viewBox.cy || 0) + 20} | |
| > | |
| Reactions | |
| </tspan> | |
| </text> | |
| ); | |
| }} |
ref https://linear.app/ghost/issue/DES-1085/update-shade-to-be-used-in-activitypub