Skip to content

Conversation

@lsc2159
Copy link
Member

@lsc2159 lsc2159 commented Feb 12, 2025

  • Add edit page [id:1,2,3]
  • Basic screen configuration for simple code operation understanding
  • Implementing dashboard, configuring components for expansion (Refer to the photo)
  • When I connect to the API, Create a services/featureFlags.ts file to change it to handle actual API requests
스크린샷 2025-02-12 오후 4 00 29 스크린샷 2025-02-12 오후 4 02 24 스크린샷 2025-02-12 오후 5 26 06 스크린샷 2025-02-12 오후 5 26 15

@lsc2159 lsc2159 added the feature New Feature label Feb 12, 2025
@lsc2159 lsc2159 requested review from HakSooDev and tjrmswo February 12, 2025 07:11
@lsc2159 lsc2159 self-assigned this Feb 12, 2025
Copy link
Contributor

@HakSooDev HakSooDev left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다.

SelectValue,
} from '@/components/ui/select';

type FeatureFlag = {
Copy link
Contributor

Choose a reason for hiding this comment

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

@lsc2159
반복되는 타입들, Variation, FeatureFlagsrc/types 에 넣으면 매번 선언 하지않아도 될것 같습니다.

const router = useRouter();
const id = params.id as string;

const originalFeatureFlag = mockFeatureFlags.find((flag) => flag.id === id);
Copy link
Contributor

Choose a reason for hiding this comment

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

개인적으로는 targetFeatureFlag 가 조금 더 가독성이 좋지않을까 싶습니다

const id = params.id as string;

const originalFeatureFlag = mockFeatureFlags.find((flag) => flag.id === id);
const [featureFlag, setFeatureFlag] = useState<FeatureFlag | undefined>(
Copy link
Contributor

Choose a reason for hiding this comment

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

여기서 살짝 null 이랑 undefined 에 차이가 있는데 보통 state 들은 undefined 보다는 null 로 표기를 해서 임의적으로 placeholder 을 놔두는게 나중에 조금 더 디버깅이나 타입 관리하기 쉬운편인것 같습니다.

  const targetFeatureFlag = mockFeatureFlags.find((flag) => flag.id === id) ?? null;

  const [featureFlag, setFeatureFlag] = useState<FeatureFlag | null>(
    originalFeatureFlag ?? null,
  );

Ref: https://stackoverflow.com/questions/5076944/what-is-the-difference-between-null-and-undefined-in-javascript

};

const handleCancel = () => {
setFeatureFlag(
Copy link
Contributor

Choose a reason for hiding this comment

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

따로 destructure 안하고 아래처럼 해도 충분할것 같습니다.

 setFeatureFlag(originalFeatureFlag);

</div>
</div>

<FeatureFlagDetails
Copy link
Contributor

Choose a reason for hiding this comment

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

컴포넌트 잘 나누신것 같습니다.

isEditing,
setFeatureFlag,
}: VariationsTableProps) {
// Variations Change
Copy link
Contributor

Choose a reason for hiding this comment

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

function naming 이 다 좋아서 굳이 코멘트들은 없어도 괜찮을것 같습니다.
다 지우는게 좋을것 같습니다.


// Variation Delete
const handleDeleteVariation = (index: number) => {
const newVariations = featureFlag.variations.filter((_, i) => i !== index);
Copy link
Contributor

Choose a reason for hiding this comment

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

newVariations 보다 updatedVariations 가 이해가 조금 더 빠르게 될것 같습니다.

</TableBody>
</Table>

{/* Add Variation Btn */}
Copy link
Contributor

Choose a reason for hiding this comment

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

이 코멘트도 지워도 괜찮을것 같습니다.

onValueChange={(value) =>
setFeatureFlag({
...featureFlag,
type: value as FeatureFlag['type'],
Copy link
Contributor

Choose a reason for hiding this comment

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

타입스크립트 공부 많이하셨군요.
as FeatureFlag['type'] 사용 잘 하신것 같습니다.

여기서 조금 UX 관련해서 타입이 바뀌면 variations 도 다 없애거나 default 값들로 리셋을 해야하지 않을까 생각이 드는데 일단 이건 급한건 아니니깐 하셔도 되고 안하셔도 괜찮습니다 .

</TableRow>
</TableHeader>
<TableBody>
{featureFlag.variations.map((variation, index) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

variationkey는 유니크하니까, 여기에서 index보다 key를 사용하는 게 어떨까 생각이 드네요. 다만, 이렇게 할려면 add variation 시 key를 어떻게 유니크하게 생성할지, 그리고 UX적으로 어떻게 풀어낼지 고민해봐야 할 것 같아요.

@lsc2159
Copy link
Member Author

lsc2159 commented Feb 17, 2025

variationkey값을 UUID로 사용해볼까 생각을 해봤습니다. 혹시 몰라서 key값 부분은 아직 index로 나뒀습니다.

@HakSooDev
Copy link
Contributor

variationkey값을 UUID로 사용해볼까 생각을 해봤습니다. 혹시 몰라서 key값 부분은 아직 index로 나뒀습니다.

넵 좋습니다 일단 그 부분은 나중에 더 덧대기로 합시다.

Copy link
Contributor

@tjrmswo tjrmswo left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

@HakSooDev HakSooDev merged commit ac8dd1d into main Feb 23, 2025
2 of 3 checks passed
@HakSooDev HakSooDev deleted the feat/edit-ui branch February 23, 2025 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New Feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants