[FEAT] Marker 컴포넌트, 스토리북 #19
Conversation
WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
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 (
|
|
Update: 2024년 10월 23일 04시 16분 15초 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
src/components/common/Marker/index.tsx (1)
9-16: Consider adding error boundaries or fallback UIThe component might fail silently if an invalid categoryName is provided or if the Icon component fails to load.
Consider implementing:
- PropTypes or TypeScript union type for valid category names
- Error boundary for catching Icon component failures
- Fallback UI when icon loading fails
Would you like me to help implement these safety measures?
src/components/common/Marker/Marker.stories.tsx (2)
1-13: Document the reason for ESLint disable commentConsider adding a brief comment explaining why the
no-restricted-exportsrule needs to be disabled here. This helps future maintainers understand if this is a Storybook requirement or a specific project need.-/* eslint-disable no-restricted-exports */ +/* eslint-disable no-restricted-exports -- Required for Storybook default export pattern */
19-29: Simplify the Basic story render functionThe render function can be simplified since it's just spreading args without any additional logic.
export const Basic: Story = { args: { categoryName: 'restaurant', }, argTypes: { categoryName: { control: { type: 'select' }, options: categoryNames }, }, - render: (args) => { - return <Marker {...args} />; - }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
src/assets/icons/marker.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
- src/assets/icons/index.tsx (2 hunks)
- src/components/common/Marker/Marker.stories.tsx (1 hunks)
- src/components/common/Marker/Marker.types.ts (1 hunks)
- src/components/common/Marker/index.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
src/components/common/Marker/Marker.types.ts (1)
1-16: LGTM! Clean type definition with good practices.The type definition is well-structured, using proper TypeScript practices:
- Correctly extends button props using ComponentPropsWithoutRef
- Uses union type for strict category names
- Includes JSDoc documentation
src/components/common/Marker/index.tsx (2)
12-12: Validate categoryName propThe component doesn't validate if the provided categoryName is valid for the Icon component.
Let's check what icon names are supported:
#!/bin/bash # Search for Icon component implementation to see supported names rg -t typescript "export type IconName" src/components/common/Icon/
1-6: Verify Props type definitionThe Props type is imported but not visible in the current context. Please ensure it properly defines the
categoryNameprop and documents any additional props that can be spread onto the button element.✅ Verification successful
Let me try a different approach to locate the Props type definition.
These commands will help us:
- Locate the types file specifically
- View its contents if found
- Find any type definitions in the Marker directory
- Check for the types file with different extensions
This will give us a comprehensive view of where and how the Props type is defined.
Props type definition is well-defined and properly documented
The Props type is correctly defined in
Marker.types.tswith:
- A properly documented
categoryNameprop with specific string literal types- Extends ComponentPropsWithoutRef<'button'> to allow spreading button props
- Clear JSDoc documentation for the categoryName prop
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Props type definition rg -t typescript "type Props" src/components/common/Marker/Length of output: 104
Script:
#!/bin/bash # Search for Props type definition using fd to find the types file first fd "Marker.types.ts" src/ # If the file exists, let's see its content fd "Marker.types.ts" src/ --exec cat {}Length of output: 412
Script:
#!/bin/bash # As a fallback, let's also check for any typescript interface or type definitions in Marker directory rg "interface|type" src/components/common/Marker/Length of output: 591
Script:
#!/bin/bash # Let's also verify if the file extension might be different fd "Marker.types" src/Length of output: 67
src/assets/icons/index.tsx (2)
19-19: LGTM! Import statement follows conventions.The Marker SVG import follows the established naming and import conventions used for other icons in the file.
42-42: LGTM! Icons object entry is consistent.The marker icon is properly added to the Icons object following the camelCase naming convention used for other entries.
Let's verify the Marker SVG file exists:
✅ Verification successful
Let me try a different approach to verify the existence of the marker SVG file.
This will help us verify both the existence of the marker.svg file and understand the icons directory structure.
The marker icon file exists and is properly placed in the icons directory
The verification confirms that
marker.svgexists atsrc/assets/icons/marker.svgalongside other SVG icons that are part of the Icons object. The file structure and naming convention are consistent with the codebase organization.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the marker.svg file exists in the icons directory fd marker.svg --type f --parent-dir src/assets/iconsLength of output: 323
Script:
#!/bin/bash # Search for marker.svg file in the repository fd "marker.svg" # Also let's check the content of the icons directory to understand the structure fd . "src/assets/icons" --type fLength of output: 843
| export type Props = { | ||
| /** | ||
| * category name to be displayed. | ||
| */ | ||
| categoryName: | ||
| | 'restaurant' | ||
| | 'cafe' | ||
| | 'bar' | ||
| | 'shopping' | ||
| | 'travel' | ||
| | 'public' | ||
| | 'hospital' | ||
| | 'other'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing type safety and reusability.
A few suggestions to improve the type definition:
- Extract categories to a separate constant/enum for reusability
- Enhance the JSDoc documentation with example usage
- Consider adding a default category
Here's how you could improve it:
import { ComponentPropsWithoutRef } from 'react';
+export const MARKER_CATEGORIES = {
+ RESTAURANT: 'restaurant',
+ CAFE: 'cafe',
+ BAR: 'bar',
+ SHOPPING: 'shopping',
+ TRAVEL: 'travel',
+ PUBLIC: 'public',
+ HOSPITAL: 'hospital',
+ OTHER: 'other',
+} as const;
+
+export type MarkerCategory = typeof MARKER_CATEGORIES[keyof typeof MARKER_CATEGORIES];
+
export type Props = {
/**
* category name to be displayed.
+ * @example
+ * ```tsx
+ * <Marker categoryName="restaurant" />
+ * ```
*/
- categoryName:
- | 'restaurant'
- | 'cafe'
- | 'bar'
- | 'shopping'
- | 'travel'
- | 'public'
- | 'hospital'
- | 'other';
+ categoryName: MarkerCategory;
} & ComponentPropsWithoutRef<'button'>;This approach:
- Makes categories reusable across the application
- Provides better type safety with const assertions
- Improves documentation with usage examples
- Makes it easier to maintain categories in one place
| export const Marker = ({ categoryName, ...props }: Props) => { | ||
| return ( |
There was a problem hiding this comment.
Add accessibility attributes to the button
The button lacks accessibility attributes which are crucial for screen readers.
Add an appropriate aria-label:
-<button className="relative inline-block" {...props}>
+<button
+ aria-label={`${categoryName} location marker`}
+ className="relative inline-block"
+ {...props}
+>📝 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.
| export const Marker = ({ categoryName, ...props }: Props) => { | |
| return ( | |
| export const Marker = ({ categoryName, ...props }: Props) => { | |
| return ( | |
| <button | |
| aria-label={`${categoryName} location marker`} | |
| className="relative inline-block" | |
| {...props} | |
| > |
| <Icon | ||
| name={categoryName} | ||
| size={35} | ||
| className={`absolute left-[51%] ${categoryName == 'restaurant' ? 'top-[55%]' : 'top-1/2'} -translate-x-1/2 -translate-y-1/2`} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting magic numbers and improving maintainability
The component has several hard-coded values that could be made more maintainable:
- Icon size of 35
- Left position of 51%
- Top position of 55%
Consider creating constants or theme variables:
+const ICON_SIZE = 35;
+const ICON_POSITIONS = {
+ default: { left: '51%', top: '50%' },
+ restaurant: { left: '51%', top: '55%' }
+} as const;
+
export const Marker = ({ categoryName, ...props }: Props) => {
+ const position = categoryName === 'restaurant'
+ ? ICON_POSITIONS.restaurant
+ : ICON_POSITIONS.default;
+
return (
<button className="relative inline-block" {...props}>
<MarkerIcon />
<Icon
name={categoryName}
- size={35}
- className={`absolute left-[51%] ${categoryName == 'restaurant' ? 'top-[55%]' : 'top-1/2'} -translate-x-1/2 -translate-y-1/2`}
+ size={ICON_SIZE}
+ className={`absolute left-[${position.left}] top-[${position.top}] -translate-x-1/2 -translate-y-1/2`}
/>Committable suggestion was skipped due to low confidence.
| export const ALL: Story = { | ||
| render: () => { | ||
| return ( | ||
| <> | ||
| {categoryNames.map((item) => ( | ||
| <Marker key={item} categoryName={item} /> | ||
| ))} | ||
| </> | ||
| ); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the ALL story with description and layout
Add a description for the story and improve the layout to better showcase all variants.
export const ALL: Story = {
+ parameters: {
+ docs: {
+ description: {
+ story: 'Displays all available marker categories in a grid layout.',
+ },
+ },
+ },
render: () => {
return (
- <>
+ <div style={{ display: 'grid', gridTemplateColumns: 'repeat(4, 1fr)', gap: '1rem' }}>
{categoryNames.map((item) => (
<Marker key={item} categoryName={item} />
))}
- </>
+ </div>
);
},
};📝 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.
| export const ALL: Story = { | |
| render: () => { | |
| return ( | |
| <> | |
| {categoryNames.map((item) => ( | |
| <Marker key={item} categoryName={item} /> | |
| ))} | |
| </> | |
| ); | |
| }, | |
| }; | |
| export const ALL: Story = { | |
| parameters: { | |
| docs: { | |
| description: { | |
| story: 'Displays all available marker categories in a grid layout.', | |
| }, | |
| }, | |
| }, | |
| render: () => { | |
| return ( | |
| <div style={{ display: 'grid', gridTemplateColumns: 'repeat(4, 1fr)', gap: '1rem' }}> | |
| {categoryNames.map((item) => ( | |
| <Marker key={item} categoryName={item} /> | |
| ))} | |
| </div> | |
| ); | |
| }, | |
| }; |
| const categoryNames: Array< | ||
| 'restaurant' | 'cafe' | 'bar' | 'shopping' | 'travel' | 'public' | 'hospital' | 'other' | ||
| > = ['restaurant', 'cafe', 'bar', 'shopping', 'travel', 'public', 'hospital', 'other']; |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Now I have enough information to provide the final response:
The review suggestion is valid and important
The category names are currently defined in two places:
- As a union type in
src/components/common/Marker/Marker.types.ts - As an array in
src/components/common/Marker/Marker.stories.tsx
Moving these categories to a shared constants file would be beneficial because:
- It would eliminate the duplication between the stories and types files
- The shared constant could be used to derive both the TypeScript type and the array of values
- Any future category changes would only need to be updated in one place
Example structure for the proposed constants file:
// src/constants/categories.ts
export const MARKER_CATEGORIES = ['restaurant', 'cafe', 'bar', 'shopping', 'travel', 'public', 'hospital', 'other'] as const;
export type MarkerCategory = typeof MARKER_CATEGORIES[number];🔗 Analysis chain
Consider moving category names to a shared constants file
The category names array should be moved to a shared constants file (e.g., src/constants/categories.ts) since these values are likely needed by both the Marker component and its stories. This ensures consistency and makes maintenance easier.
Let's check if these categories are defined elsewhere:
Let me search for any usage of these category names individually to ensure we're not missing any other definitions or usages.
Let me check the content of Marker.types.ts to see how these categories are defined there, as it seems to be the main type definition file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for category name definitions across the codebase
rg -l "restaurant.*cafe.*bar.*shopping.*travel.*public.*hospital.*other"
Length of output: 122
Script:
#!/bin/bash
# Search for individual category names and their usage in type definitions or constants
rg -A 2 -B 2 "type.*Category.*=.*(" || true
rg -A 2 -B 2 "interface.*Category.*{" || true
rg -A 2 -B 2 "enum.*Category.*{" || true
# Search for individual category names in quotes
echo "Searching for individual categories:"
for category in restaurant cafe bar shopping travel public hospital other; do
echo -e "\nFiles containing '$category':"
rg -l "'$category'" || true
rg -l "\"$category\"" || true
done
Length of output: 2926
Script:
#!/bin/bash
# Check the content of Marker.types.ts
cat src/components/common/Marker/Marker.types.ts
Length of output: 350
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/constants/categories.ts (1)
1-10: Add JSDoc documentation for better maintainability.The categories are well-defined and the
as constassertion ensures type safety. Consider adding documentation to explain the purpose and usage of these categories.+/** + * Predefined categories for map markers. + * Used by the Marker component to determine the icon and style of map markers. + */ export const MARKER_CATEGORIES = [
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/components/common/Marker/Marker.stories.tsx (1 hunks)
- src/components/common/Marker/Marker.types.ts (1 hunks)
- src/components/common/Marker/index.tsx (1 hunks)
- src/constants/categories.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/common/Marker/Marker.stories.tsx
- src/components/common/Marker/Marker.types.ts
- src/components/common/Marker/index.tsx
🧰 Additional context used
| export const MARKER_CATEGORIES = [ | ||
| 'restaurant', | ||
| 'cafe', | ||
| 'bar', | ||
| 'shopping', | ||
| 'travel', | ||
| 'public', | ||
| 'hospital', | ||
| 'other', | ||
| ] as const; | ||
| export type MarkerCategory = (typeof MARKER_CATEGORIES)[number]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the category structure for better maintainability.
While the current implementation is functional, consider structuring the categories as an object with metadata for better maintainability and flexibility.
interface CategoryMetadata {
label: string;
icon?: string;
color?: string;
}
export const MARKER_CATEGORIES = {
restaurant: { label: '음식점' },
cafe: { label: '카페' },
bar: { label: '바' },
shopping: { label: '쇼핑' },
travel: { label: '여행' },
public: { label: '공공시설' },
hospital: { label: '병원' },
other: { label: '기타' },
} as const;
export type MarkerCategory = keyof typeof MARKER_CATEGORIES;This structure would:
- Allow for additional metadata per category
- Provide human-readable labels
- Support future extensions (icons, colors, etc.)
- Maintain type safety
관련 이슈
close #13
📑 작업 내용
💬 리뷰 중점 사항/기타 참고 사항
Summary by CodeRabbit
New Features
Markericon for enhanced visual representation in the application.Markercomponent that allows users to select various categories (e.g., restaurant, cafe, bar) through a button interface.Markercomponent to visualize its usage in different scenarios.MARKER_CATEGORIESfor predefined category options.Bug Fixes
Documentation
Propstype for theMarkercomponent.Chores
Markercomponent for better maintainability and usability.