Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEAT] Marker 컴포넌트, 스토리북 #19

Merged
merged 7 commits into from
Oct 23, 2024
Merged

[FEAT] Marker 컴포넌트, 스토리북 #19

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a14492b
feat : add marker svg
keemsebin Oct 22, 2024
230e5b6
chore : remove gitkeep
keemsebin Oct 22, 2024
c4064a0
feat : configure marker props type
keemsebin Oct 22, 2024
fe6d819
feat : add Marker component
keemsebin Oct 22, 2024
3d2e9a0
feat : create Marker storybook
keemsebin Oct 22, 2024
cab22ef
feat : create category constant
keemsebin Oct 22, 2024
9e98806
refactor : move icon size and position to constants
keemsebin Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/assets/icons/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import KakaoLogin from './kakao-login.svg?react';
import Link from './link.svg?react';
import Location from './location.svg?react';
import Map from './map.svg?react';
import Marker from './marker.svg?react';
import NaverLogin from './naver-login.svg?react';
import Other from './other.svg?react';
import Public from './public.svg?react';
Expand All @@ -38,6 +39,7 @@ export const Icons = {
link: Link,
map: Map,
location: Location,
marker: Marker,
kakaoLogin: KakaoLogin,
naverLogin: NaverLogin,
findy1: Findy1,
Expand Down
18 changes: 18 additions & 0 deletions src/assets/icons/marker.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Empty file removed src/components/common/.gitkeep
View file
Open in desktop
Empty file.
41 changes: 41 additions & 0 deletions src/components/common/Marker/Marker.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/* eslint-disable no-restricted-exports */
import type { Meta, StoryObj } from '@storybook/react';

import { Marker } from '.';

const meta = {
title: 'components/common/Marker',
component: Marker,
tags: ['autodocs'],
} satisfies Meta<typeof Marker>;

export default meta;
type Story = StoryObj<typeof Marker>;

const categoryNames: Array<
'restaurant' | 'cafe' | 'bar' | 'shopping' | 'travel' | 'public' | 'hospital' | 'other'
> = ['restaurant', 'cafe', 'bar', 'shopping', 'travel', 'public', 'hospital', 'other'];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. It would eliminate the duplication between the stories and types files
  2. The shared constant could be used to derive both the TypeScript type and the array of values
  3. 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


export const Basic: Story = {
args: {
categoryName: 'restaurant',
},
argTypes: {
categoryName: { control: { type: 'select' }, options: categoryNames },
},
render: (args) => {
return <Marker {...args} />;
},
};

export const ALL: Story = {
render: () => {
return (
<>
{categoryNames.map((item) => (
<Marker key={item} categoryName={item} />
))}
</>
);
},
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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>
);
},
};

16 changes: 16 additions & 0 deletions src/components/common/Marker/Marker.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { ComponentPropsWithoutRef } from 'react';

export type Props = {
/**
* category name to be displayed.
*/
categoryName:
| 'restaurant'
| 'cafe'
| 'bar'
| 'shopping'
| 'travel'
| 'public'
| 'hospital'
| 'other';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider enhancing type safety and reusability.

A few suggestions to improve the type definition:

  1. Extract categories to a separate constant/enum for reusability
  2. Enhance the JSDoc documentation with example usage
  3. 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

} & ComponentPropsWithoutRef<'button'>;
18 changes: 18 additions & 0 deletions src/components/common/Marker/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import MarkerIcon from '@/assets/icons/marker.svg';

import { Props } from './Marker.types';

import { Icon } from '../Icon';

export const Marker = ({ categoryName, ...props }: Props) => {
return (
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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}
>

<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`}
/>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

</button>
);
};
Loading