feat(studio,web): add dosb icons to groups #168
Conversation
mheob
added
enhancement
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/studio/schemas/documents/group.tsxOops! Something went wrong! :( ESLint: 9.14.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@mheob/prettier-config' imported from /prettier.config.mjs WalkthroughThis pull request introduces a comprehensive refactoring and enhancement of the project's shared resources, focusing on icon management, package structure, and utility functions. The changes include renaming the shared package, adding DOSB (Deutscher Olympischer Sportbund) sports icons, updating import paths across multiple components, and consolidating utility functions. The modifications affect both the web and studio applications, introducing more standardized and flexible icon and class name handling. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
✨ Finishing Touches
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 (5)
apps/web/src/components/ui/news-article-preview.tsx (1)
1-1: LGTM! Consider memoizing complex className conditionals.The import change aligns with the codebase-wide refactoring. However, since the component uses complex className conditionals, consider memoizing them to optimize performance.
+ const articleClassName = useMemo( + () => cn('bg-background group flex flex-col rounded-xl shadow-lg', { + 'md:grid md:grid-cols-2': columns === 2, + }), + [columns] + ); return ( <article - className={cn('bg-background group flex flex-col rounded-xl shadow-lg', { - 'md:grid md:grid-cols-2': columns === 2, - })} + className={articleClassName} >apps/web/src/app/_home/hero.tsx (1)
1-1: LGTM! Consider extracting complex className combinations.The import change aligns with the codebase-wide refactoring. However, the complex responsive className combinations could be extracted for better readability.
+ const imageClassName = cn( + 'absolute', + 'bottom-20 end-[10%] start-[10%] w-[80%]', + 'md:bottom-10 md:end-auto md:start-1/2 md:w-[40%]', + '3xl:bottom-10 3xl:end-auto 3xl:start-1/2 3xl:w-[33%]', + ); + const navClassName = cn( + 'flex text-white', + 'w-full items-end justify-around justify-self-end py-10', + 'md:flex-col md:justify-center md:gap-10', + ); return ( // ... <Image - className={cn( - 'absolute', - 'bottom-20 end-[10%] start-[10%] w-[80%]', - 'md:bottom-10 md:end-auto md:start-1/2 md:w-[40%]', - '3xl:bottom-10 3xl:end-auto 3xl:start-1/2 3xl:w-[33%]', - )} + className={imageClassName} // ... /> <nav - className={cn( - 'flex text-white', - 'w-full items-end justify-around justify-self-end py-10', - 'md:flex-col md:justify-center md:gap-10', - )} + className={navClassName} >apps/studio/schemas/sections/block-content.ts (1)
45-49: Consider using a more specific type instead of Record<string, any>.While the current implementation works, using
Record<string, any>reduces type safety. Consider creating a specific interface for the value parameter:interface BlockContentValue { blocks: Array<PortableTextTextBlock<PortableTextSpan>>; }This would provide better type checking and documentation.
apps/web/src/types/sanity.types.generated.ts (2)
872-889: Consider standardizing sport names to English.While the DOSB icon integration looks good, there's an inconsistency in language usage. Some sports use German terms (e.g., 'Fussball') while others use English terms (e.g., 'Badminton').
Consider standardizing all sport names to English for consistency:
icon: | 'Badminton' | 'Bodenturnen' | 'Cheerleading' | 'Fitness' - | 'Fussball' + | 'Football' | 'Gymnastik' | 'Jujutsu' | 'Pilates' | 'RopeSkipping' | 'SportInGebaeuden' | 'Sportakrobatik' | 'StepAerobic' | 'Taekwondo' | 'Tanzen' | 'Turnen' | 'Wandern' | 'Yoga';
1520-1537: Consider using a shared type for DOSB icons.The icon type is duplicated between
GroupandHomePageGroupsQueryResult. To improve maintainability, consider extracting it into a shared type.Example refactor:
export type DosbIconName = | 'Badminton' | 'Bodenturnen' | 'Cheerleading' | 'Fitness' | 'Fussball' | 'Gymnastik' | 'Jujutsu' | 'Pilates' | 'RopeSkipping' | 'SportInGebaeuden' | 'Sportakrobatik' | 'StepAerobic' | 'Taekwondo' | 'Tanzen' | 'Turnen' | 'Wandern' | 'Yoga'; // Then use it in both types: export type Group = { // ...other properties icon: DosbIconName; }; export type HomePageGroupsQueryResult = Array<{ title: string; icon: DosbIconName; }>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (42)
.cspell.json(2 hunks)apps/studio/package.json(1 hunks)apps/studio/schema.json(2 hunks)apps/studio/schemas/documents/group.tsx(2 hunks)apps/studio/schemas/sections/block-content.ts(1 hunks)apps/web/package.json(1 hunks)apps/web/src/app/_home/group-card.tsx(2 hunks)apps/web/src/app/_home/hero.tsx(1 hunks)apps/web/src/app/_home/testimonial-group.tsx(1 hunks)apps/web/src/app/layout.tsx(1 hunks)apps/web/src/app/news/[category]/[slug]/page.tsx(1 hunks)apps/web/src/components/layout/main-nav/desktop.tsx(1 hunks)apps/web/src/components/layout/main-nav/mobile.tsx(1 hunks)apps/web/src/components/section/contact-persons.tsx(1 hunks)apps/web/src/components/section/hero.tsx(1 hunks)apps/web/src/components/section/newsletter.tsx(1 hunks)apps/web/src/components/ui/arrow-button-group.tsx(1 hunks)apps/web/src/components/ui/badge.tsx(1 hunks)apps/web/src/components/ui/breadcrumb.tsx(1 hunks)apps/web/src/components/ui/button.tsx(1 hunks)apps/web/src/components/ui/card.tsx(1 hunks)apps/web/src/components/ui/contact-persons.tsx(1 hunks)apps/web/src/components/ui/counter.tsx(1 hunks)apps/web/src/components/ui/drawer.tsx(1 hunks)apps/web/src/components/ui/form.tsx(1 hunks)apps/web/src/components/ui/input.tsx(1 hunks)apps/web/src/components/ui/label.tsx(1 hunks)apps/web/src/components/ui/navigation-menu.tsx(1 hunks)apps/web/src/components/ui/news-article-preview.tsx(1 hunks)apps/web/src/components/ui/pricing-card.tsx(1 hunks)apps/web/src/components/ui/section-header.tsx(1 hunks)apps/web/src/components/ui/select.tsx(1 hunks)apps/web/src/components/ui/separator.tsx(1 hunks)apps/web/src/components/ui/textarea.tsx(1 hunks)apps/web/src/types/sanity.types.generated.ts(3 hunks)packages/shared/package.json(1 hunks)packages/shared/src/env.ts(0 hunks)packages/shared/src/icons/dosb.tsx(1 hunks)packages/shared/src/icons/index.ts(1 hunks)packages/shared/src/index.ts(1 hunks)packages/shared/src/utils/index.ts(1 hunks)packages/shared/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/shared/src/env.ts
✅ Files skipped from review due to trivial changes (19)
- apps/web/src/components/ui/section-header.tsx
- apps/web/src/components/ui/textarea.tsx
- packages/shared/src/utils/index.ts
- apps/web/src/components/ui/counter.tsx
- apps/web/src/components/ui/separator.tsx
- packages/shared/src/icons/index.ts
- apps/web/src/components/ui/breadcrumb.tsx
- apps/web/src/components/layout/main-nav/desktop.tsx
- apps/web/src/components/layout/main-nav/mobile.tsx
- apps/web/src/components/ui/navigation-menu.tsx
- apps/web/src/app/news/[category]/[slug]/page.tsx
- packages/shared/src/index.ts
- apps/web/src/components/ui/badge.tsx
- apps/web/src/components/ui/button.tsx
- apps/web/src/components/ui/form.tsx
- apps/web/src/components/section/hero.tsx
- apps/web/src/components/ui/select.tsx
- apps/web/src/components/ui/drawer.tsx
- apps/web/src/components/ui/pricing-card.tsx
🔇 Additional comments (27)
apps/web/src/components/ui/label.tsx (1)
4-4: LGTM: Import migration looks good.The change to import
cnfrom the shared library is consistent with the refactoring objectives.apps/web/src/components/section/contact-persons.tsx (1)
1-2: LGTM: Clean import change with improved formatting.The import migration and added spacing improve code organization while maintaining functionality.
apps/web/src/app/layout.tsx (1)
1-1: LGTM: Root layout import updated correctly.The shared library import change is properly implemented in this critical layout component.
apps/web/src/components/section/newsletter.tsx (2)
1-1: LGTM: Import migration completed successfully.The shared library import change is consistent with the project-wide refactoring.
Line range hint
27-41: Consider enhancing form security and validation.The newsletter form submission could benefit from additional security measures and validation:
- Add CSRF protection token
- Implement client-side email validation
- Add error handling for failed submissions
Example implementation:
<form className="z-1 max-w-(--breakpoint-sm) relative mt-16 flex items-center justify-between gap-2 rounded-md bg-white px-2 pb-2 shadow-xl md:mx-auto md:gap-6 md:px-6" method="post" + onSubmit={handleSubmit} > + <input type="hidden" name="csrf_token" value={csrf_token} /> <label aria-label="E-Mail" className="text-primary sr-only md:not-sr-only" htmlFor="email" > <AtSign /> </label> <Input className="flex-1 bg-transparent p-4 text-sm md:text-lg" id="email" name="email" placeholder="[email protected]" type="email" + required + pattern="[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,}$" /> <Button type="submit">Abonnieren</Button> + {error && <p className="text-red-500">{error}</p>} </form>✅ Verification successful
Security enhancements are indeed needed for the newsletter form
The review comment is accurate. The current implementation lacks:
- CSRF protection
- Proper form validation beyond basic HTML5 email type
- Error handling for failed submissions
The suggested implementation aligns well with the codebase's patterns and would improve the form's security and user experience.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find the complete newsletter component implementation ast-grep --pattern 'export $_ function Newsletter() { $$$ }' # Look for any form submission handlers in the newsletter component ast-grep --pattern 'function handleSubmit($$$) { $$$ }' # Search for CSRF-related code rg -i "csrf" --type ts --type tsx # Look for existing form validation patterns rg -i "validation|validate" --type ts --type tsxLength of output: 265
Script:
#!/bin/bash # Find the complete newsletter component implementation cat apps/web/src/components/section/newsletter.tsx # Search for CSRF-related code in the codebase rg -i "csrf" # Look for form validation patterns rg -i "validation|validate" # Search for form submission handling rg -i "onSubmit|handleSubmit" apps/web/src/components/section/Length of output: 13524
apps/web/src/components/ui/input.tsx (1)
1-1: LGTM! Verify shared package implementation.The import change aligns with the codebase-wide refactoring to use shared utilities. However, let's verify the shared package implementation.
✅ Verification successful
✓ Shared package implementation verified
The
cnutility in@tsgi-web/sharedis correctly implemented usingclsxandtailwind-merge, following best practices for className management in React+Tailwind projects. The import change is safe to use.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the shared package implementation of the cn utility # Expected: The implementation should be equivalent to the original # Check if the shared package exists and contains the cn utility rg -p "export.*function cn\(" packages/shared/Length of output: 129
Script:
#!/bin/bash # Get the complete implementation of cn utility from shared package echo "=== Shared package implementation ===" cat packages/shared/src/utils/cn.ts # Search for any previous implementations of cn utility echo -e "\n=== Previous implementations ===" rg -p "function cn\(" --type tsLength of output: 985
apps/web/src/components/ui/card.tsx (1)
1-1: LGTM! Import change is consistent.The import change aligns with the codebase-wide refactoring and maintains functionality across all card subcomponents.
apps/web/src/app/_home/hero.tsx (1)
1-1: Verify alignment with PR objectives.The changes show a refactoring of the
cnutility to a shared package, but the PR objective mentions adding DOSB icons to groups. Please clarify if there are additional changes pending or if the PR description needs to be updated.✅ Verification successful
Changes align with PR objectives
The PR includes both the
cnutility refactoring and DOSB icon implementation (packages/shared/src/icons/dosb.tsx), suggesting these changes are part of a broader shared package organization effort. The original concern about misalignment is resolved.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for DOSB icon-related changes # Expected: Find evidence of DOSB icon additions # Look for DOSB-related files or changes rg -i "dosb|deutscher olympischer sportbund" --type ts --type tsx # Look for icon-related changes rg -p "import.*icons?.*dosb" --type ts --type tsxLength of output: 175
Script:
#!/bin/bash # Get PR description and title gh pr view --json title,body # Search for DOSB/icon related changes with correct file types rg -t typescript -t javascriptreact -t typescriptreact -i "dosb|deutscher olympischer sportbund" # Look for icon-related changes rg -t typescript -t javascriptreact -t typescriptreact "import.*[\"'].*icons?.*dosb" # Check for any new icon components or SVG files fd -e svg -e tsx -e ts "dosb|icon" -ILength of output: 544
apps/web/src/components/ui/arrow-button-group.tsx (1)
2-2: LGTM! Import update aligns with codebase standardization.The change to import
cnfrom@tsgi-web/sharedis consistent with the broader refactoring effort to standardize utility functions.apps/web/src/components/ui/contact-persons.tsx (1)
3-3: LGTM! Import update maintains consistency.The update to import
cnfrom@tsgi-web/sharedaligns with the standardization of utility functions across the codebase.apps/web/src/app/_home/testimonial-group.tsx (1)
3-3: LGTM! Import update completes the standardization pattern.The update to import
cnfrom@tsgi-web/sharedmaintains consistency with the codebase-wide standardization of utility functions.Let's verify that all components have been updated to use the shared package:
✅ Verification successful
Import migration verification successful ✓
All components, including testimonial-group.tsx, are correctly using the standardized import from '@tsgi-web/shared'. No instances of the old import pattern remain in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no components are still using the old import path # Test: Search for any remaining imports from @/utils/cn # Expect: No results, indicating complete migration rg "from '@/utils/cn'" --type ts --type tsxLength of output: 79
Script:
#!/bin/bash # Search for any remaining imports from old path with various quote styles rg "from ['\"]@/utils/cn['\"]" --type typescript --type-add 'typescript:*.{ts,tsx}' # Also check for direct imports of the file rg "from ['\"]@/utils/cn\.ts['\"]" --type typescript --type-add 'typescript:*.{ts,tsx}' # Check current pattern usage for context rg "from ['\"]@tsgi-web/shared['\"]" --type typescript --type-add 'typescript:*.{ts,tsx}'Length of output: 2663
apps/web/src/app/_home/group-card.tsx (2)
1-1: LGTM! Props interface changes improve type safety.The change from
stringtoDosbIconNamefor the icon prop provides better type checking and ensures only valid DOSB icons can be used.Also applies to: 5-9
17-19: LGTM! Icon rendering implementation looks good.The icon is properly rendered using the DOSBIcon component with appropriate sizing and styling classes.
apps/studio/schemas/documents/group.tsx (2)
1-1: LGTM! Icon options implementation is clean and maintainable.Using
DOSB_ICONS.map()to generate the options list is a good approach that ensures consistency and maintainability.Also applies to: 74-74
62-70: LGTM! Clear and helpful description for users.The description provides a good reference to the DOSB pictograms and clear instructions for users when they need additional icons.
packages/shared/src/icons/dosb.tsx (3)
5-24: LGTM! Well-structured icon name definitions.The DOSB_ICONS constant provides a centralized list of available icons with proper TypeScript const assertions for type safety.
26-62: LGTM! DOSBIcon component implementation is clean and reusable.The component:
- Properly extends SVG props
- Uses proper SVG attributes
- Implements consistent sizing and styling
- Uses currentColor for theming flexibility
64-431: LGTM! Icon implementations follow SVG best practices.The individual icon components:
- Use consistent fill and fillRule attributes
- Properly handle paths and groups
- Follow accessibility best practices
packages/shared/tsconfig.json (1)
2-5: Improved TypeScript configuration for React development.Good improvements:
- Using React-specific TypeScript configuration
- Added path alias for cleaner imports
Let's verify the path alias usage:
✅ Verification successful
Path alias "@/*" is correctly configured and actively used
The path alias is properly configured and extensively used throughout the codebase for clean and consistent imports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the path alias is being used correctly # Test: Search for @/* imports rg "from ['|\"]@/"Length of output: 24954
packages/shared/package.json (2)
2-2: Package renamed and exports configured correctly.Good improvements:
- More concise package name
- Explicit root export configuration
Also applies to: 9-9
16-19: Added essential React dependencies and utilities.The added dependencies align with React development needs and include utilities for class name handling.
Let's verify the dependency versions and usage:
Also applies to: 24-25
✅ Verification successful
Dependencies are properly integrated and utilized
The dependencies are correctly configured and actively used throughout the codebase. The utilities (clsx, tailwind-merge) are properly combined in a utility function, and React is extensively used in component implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check dependency versions and usage # Test 1: Check if clsx and tailwind-merge are used together rg "import.*(?:clsx|tailwind-merge)" # Test 2: Verify React usage rg "import.*(?:react|react-dom)"Length of output: 11238
apps/studio/package.json (1)
19-19: Updated shared package dependency name.The dependency has been correctly updated to use the new package name.
Let's verify the dependency update across the workspace:
✅ Verification successful
Dependency update verification complete
The new package name
@tsgi-web/sharedis consistently used across the workspace with no remaining references to the old package name.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining old package references # Test: Search for old package name references rg "@tsg-irlich-web/shared"Length of output: 27
Script:
#!/bin/bash # Search for new package name references echo "=== Searching for new package name ===" rg "@tsgi-web/shared" echo -e "\n=== Checking package.json files ===" fd package.json --exec cat {} echo -e "\n=== Checking import/require statements ===" ast-grep --pattern 'import $_ from "@tsgi-web/shared"' ast-grep --pattern 'require("@tsgi-web/shared")'Length of output: 7134
.cspell.json (1)
30-30: LGTM: Dictionary additions align with DOSB icons integrationThe new words "Cheerleading", "dosb", and "tsgi" are appropriate additions to support the DOSB icons feature and maintain consistent spelling across the codebase.
Also applies to: 34-34, 81-81
apps/web/package.json (1)
26-26: LGTM: Workspace dependency properly configuredThe addition of
@tsgi-web/sharedas a workspace dependency is correctly configured and aligns with the package structure changes.apps/studio/schema.json (2)
605-625: LGTM: Well-structured season field additionThe new
seasonfield intrainingTimetype is well-designed with appropriate values ("yearly", "summer", "winter") and is correctly marked as required.
5007-5071: LGTM: Comprehensive DOSB icons integrationThe addition of DOSB sport icons provides a comprehensive list of supported sports activities, properly formatted as string enum values. The list includes common sports and activities like Badminton, Cheerleading, Fitness, etc.
apps/web/src/types/sanity.types.generated.ts (1)
123-124: LGTM! Improved type definitions for TrainingTime.The changes enhance the type definitions by:
- Adding season support ('yearly' | 'summer' | 'winter')
- Switching to English weekday values for better internationalization
closes #103
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the release notes:
New Features
Dependency Updates
@tsg-irlich-web/sharedto@tsgi-web/sharedType Improvements
Chores