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.

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/#16] 공통 네브바 제작 #37

Merged
merged 4 commits into from
Feb 4, 2025
Merged

[Feat/#16] 공통 네브바 제작 #37

merged 4 commits into from
Feb 4, 2025

Conversation

lgrin-byte
Copy link
Member

💡 변경사항 & 이슈

공통 네브바 및 3가지 네브바 구현

✍️ 관련 설명

이미지, 텍스트 들어가게 하려고 navbarButton컴포넌트으로 분리했음!
더 확장될 것 같지 않아서 컴포넌트 3개로 분리해서 필요한 화면에 가져다가 쓰면 될 듯!

⭐️ Review point

  • 로직 개선점
  • 이외 의견이나 피드백

📷 Demo

스크린샷 2025-02-02 오후 2 59 05 스크린샷 2025-02-02 오후 2 58 45 스크린샷 2025-02-02 오후 2 58 19

@lgrin-byte lgrin-byte requested a review from sikkzz as a code owner February 2, 2025 06:02
Copy link

github-actions bot commented Feb 2, 2025

🧷 Storybook: https://6798a432c75b75bf6b81bc26-dwahorbzwa.chromatic.com/

⏰ Update: 2025년 02월 04일 21시 37분

Copy link
Collaborator

@sikkzz sikkzz left a comment

Choose a reason for hiding this comment

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

구현하느라 고생했어!! 근데 더 좋은 방법들이 있어서 몇가지 달아볼게

우선 이런 컴포넌트는 보통 compound 패턴을 사용해서 많이 구현하는 거 같아

일단 우리 디자인 상으로는 navbar의 종류가 3가지로 딱 fix 되어있고 바뀔 염려가 거의 없어보이지만 항상 확장성은 열어두고 구현하는 게 좋거든. navbar의 종류가 더 추가될 경우나, 해당 navbar의 종류에서도 로고나 버튼의 종류가 여러개가 되는 경우 등 변동의 여지가 추후에 생길 수도 있자나?

그 경우를 대비해서 내부 요소가 Navbar에 종속되는게 아니라 별개의 요소로 동작하는게 좋기 때문에 compound 패턴이 어울리는 거라고 생각해! 그리고 내가 올려준 코드 보면 결국 compound 패턴으로 navbar의 틀만 정해주고 나머지 내부 요소는 이미 내가 구현한 컴포넌트들로 다 해결이 가능하니까 훨씬 구현하기 편하고 간단하지.

여기서 확장성이 생기면 내부 컴포넌트들을 더 확장하면 되는거니까 이부분 또한 생산성의 이점이라고 생각해!

그래서 내가 compound 패턴으로 구현한 케이스를 커밋 넣어볼테니까 코드 한번 확인해보고 뭔가 다른 의견이 있거나 더 좋은 방법이 있으면 의견 부탁할게!

src/components/ui/Navbar/Navbar.module.css Outdated Show resolved Hide resolved
@sikkzz sikkzz assigned sikkzz and lgrin-byte and unassigned sikkzz Feb 2, 2025
@sikkzz sikkzz added the Feature new feature label Feb 2, 2025
@sikkzz
Copy link
Collaborator

sikkzz commented Feb 2, 2025

구현하느라 고생했어!! 근데 더 좋은 방법들이 있어서 몇가지 달아볼게

우선 이런 컴포넌트는 보통 compound 패턴을 사용해서 많이 구현하는 거 같아

일단 우리 디자인 상으로는 navbar의 종류가 3가지로 딱 fix 되어있고 바뀔 염려가 거의 없어보이지만 항상 확장성은 열어두고 구현하는 게 좋거든. navbar의 종류가 더 추가될 경우나, 해당 navbar의 종류에서도 로고나 버튼의 종류가 여러개가 되는 경우 등 변동의 여지가 추후에 생길 수도 있자나?

그 경우를 대비해서 내부 요소가 Navbar에 종속되는게 아니라 별개의 요소로 동작하는게 좋기 때문에 compound 패턴이 어울리는 거라고 생각해! 그리고 내가 올려준 코드 보면 결국 compound 패턴으로 navbar의 틀만 정해주고 나머지 내부 요소는 이미 내가 구현한 컴포넌트들로 다 해결이 가능하니까 훨씬 구현하기 편하고 간단하지.

여기서 확장성이 생기면 내부 컴포넌트들을 더 확장하면 되는거니까 이부분 또한 생산성의 이점이라고 생각해!

그래서 내가 compound 패턴으로 구현한 케이스를 커밋 넣어볼테니까 코드 한번 확인해보고 뭔가 다른 의견이 있거나 더 좋은 방법이 있으면 의견 부탁할게!

아 추가적으로 navigate 관련 기능을 내부에 안넣은거는 navigate도 navbar 컴포넌트에 종속되는거보다 navbar를 사용하는 페이지에서 routing이 일어나는게 더 맞다고 생각해서 props로 onClick을 통해 구현했어

라우팅이 일관될 수도 있지만 네이티브 라우팅을 통해 이동해야할 경우(OCR 페이지로 이동 등)도 있을꺼고 뒤로가기나 close버튼이 꼭 동일하게 작동하지 않을 수도 있을 거 같아서!

@lgrin-byte
Copy link
Member Author

오 그렇구나 좋은 의견 고마웡!! 👍

@sikkzz sikkzz merged commit 84e132d into develop Feb 4, 2025
3 checks passed
@sikkzz sikkzz deleted the feat/#16 branch February 4, 2025 12:42
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.

2 participants