[Step1] hippo: styled-components 적용하기 - #1
Conversation
WalkthroughCSS 모듈 기반 서브디렉터리 구조( Changesstyled-components 마이그레이션
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
meteorqz6
changed the title
- section > ul 이중 구조를 ul 단일 구조로 변경 (styled.section → styled.ul) - Info 내 자식 선택자(h3, p)를 RestaurantName, RestaurantDescription으로 분리 - 하드코딩 색상을 var(--grey-150)으로 교체
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/components/AddRestaurantModal.jsx (1)
6-25: ⚡ Quick win라벨 스타일은 child selector 중첩보다 명시적 컴포넌트 분리가 더 안전합니다.
현재 구조는
FormItem의 하위에label이 있다는 전제에 의존해서, 마크업이 바뀌면 스타일이 같이 깨지기 쉽습니다.FormLabel/RequiredFormLabel을 별도styled컴포넌트로 분리하면 재사용성과 변경 안정성이 좋아집니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AddRestaurantModal.jsx` around lines 6 - 25, The current implementation relies on child selector nesting where label styling is defined inside FormItem and RequiredFormItem, which is fragile if the HTML structure changes. Extract the label styling into separate styled components: create a FormLabel styled component containing the current label styles from FormItem, and a RequiredFormLabel styled component that extends FormLabel with the asterisk styling currently in RequiredFormItem. This makes the components more explicit, reusable, and less dependent on specific DOM hierarchy assumptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 142-152: The markdown code blocks in README.md starting at line
142 are missing language specification after the opening triple backticks, which
violates markdown linter rule MD040. Add a language identifier after the opening
triple backticks of the code blocks. Since these blocks show file structure, use
`text` as the language specifier (e.g., change the opening ``` to ```text).
Ensure all code blocks have proper opening and closing triple backticks with the
appropriate language specified.
In `@src/components/Header.jsx`:
- Line 46: The img element at line 46 that displays the addButton icon is
missing an alt attribute, which is required for accessibility. Since this img is
a visual decoration element for the button and the button's accessibility name
is already provided by the aria-label attribute on line 43, add an empty alt
attribute (alt="") to the img element to properly indicate that it is decorative
and should be ignored by screen readers.
In `@src/components/Modal.jsx`:
- Around line 41-43: The ModalContainer element in Modal.jsx (lines 41-43) is
missing critical accessibility attributes required for keyboard navigation and
screen reader support. Add the role="dialog" attribute to the ModalContainer to
identify it as a dialog, add aria-modal="true" to indicate it's a modal overlay,
and connect the ModalTitle to the container using aria-labelledby with matching
IDs so screen readers announce the title. Additionally, implement focus
management by ensuring keyboard focus moves into the modal on opening (typically
to the first focusable element or the title) and returns to the previously
focused element when the modal closes, which prevents focus from escaping to the
background and ensures proper context for keyboard users.
In `@src/components/RestaurantList.jsx`:
- Line 73: The Button component on line 73 is missing the type attribute, which
can cause unintended form submission behavior when the button is rendered inside
a form element. Add type="button" to the Button component to explicitly prevent
the default form submission and ensure only the onRestaurantClick handler is
executed when clicked.
---
Nitpick comments:
In `@src/components/AddRestaurantModal.jsx`:
- Around line 6-25: The current implementation relies on child selector nesting
where label styling is defined inside FormItem and RequiredFormItem, which is
fragile if the HTML structure changes. Extract the label styling into separate
styled components: create a FormLabel styled component containing the current
label styles from FormItem, and a RequiredFormLabel styled component that
extends FormLabel with the asterisk styling currently in RequiredFormItem. This
makes the components more explicit, reusable, and less dependent on specific DOM
hierarchy assumptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: db9743f4-3d8c-4676-b58e-bbc6121dca6c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
01-first-component/README.md01-styled-components/README.md02-rendering-lists/README.md03-modal/README.md04-form/README.md05-effects/README.mdREADME.mdpackage.jsonsrc/App.csssrc/App.jsxsrc/components/AddRestaurantModal.jsxsrc/components/AddRestaurantModal/AddRestaurantModal.jsxsrc/components/AddRestaurantModal/AddRestaurantModal.module.csssrc/components/CategoryFilter.jsxsrc/components/CategoryFilter/CategoryFilter.jsxsrc/components/CategoryFilter/CategoryFilter.module.csssrc/components/Header.jsxsrc/components/Header/Header.jsxsrc/components/Header/Header.module.csssrc/components/Modal.jsxsrc/components/Modal/Modal.jsxsrc/components/Modal/Modal.module.csssrc/components/RestaurantDetailModal.jsxsrc/components/RestaurantDetailModal/RestaurantDetailModal.jsxsrc/components/RestaurantDetailModal/RestaurantDetailModal.module.csssrc/components/RestaurantList.jsxsrc/components/RestaurantList/RestaurantList.jsxsrc/components/RestaurantList/RestaurantList.module.css
💤 Files with no reviewable changes (17)
- 01-first-component/README.md
- 04-form/README.md
- src/components/AddRestaurantModal/AddRestaurantModal.jsx
- src/components/RestaurantList/RestaurantList.module.css
- 03-modal/README.md
- src/components/RestaurantDetailModal/RestaurantDetailModal.jsx
- src/components/Modal/Modal.jsx
- 05-effects/README.md
- src/components/Header/Header.module.css
- src/components/Modal/Modal.module.css
- src/components/Header/Header.jsx
- 02-rendering-lists/README.md
- src/components/CategoryFilter/CategoryFilter.module.css
- src/components/RestaurantDetailModal/RestaurantDetailModal.module.css
- src/components/CategoryFilter/CategoryFilter.jsx
- src/components/AddRestaurantModal/AddRestaurantModal.module.css
- src/components/RestaurantList/RestaurantList.jsx
There was a problem hiding this comment.
자식 선택자 중첩 질문 주신 부분에 대해 제 생각도 남겨볼게요!
README에 정리해주신 의미 있는 콘텐츠는 분리하고, 단순 아이콘은 중첩하는 기준에 동의합니다.
여기에 더해서 선택자의 정밀도도 기준이 될 수 있다고 생각합니다. 자식 선택자(img { })는 부모 컴포넌트 안에 있는 모든 img 태그에 무조건 적용됩니다. 예를 들어 나중에 GnbButton 안에 알림 뱃지 같은 다른 이미지가 추가되면, 의도치 않게 그 이미지에도 같은 스타일이 적용될 수 있습니다.
그래서 자식 선택자 중첩을 결정할 때는 '콘텐츠로서 의미가 있는지'와 '구조가 바뀔 때도 스타일이 의도한 요소에만 적용되는지'를 같이 보고 상황에 따라 섞어서 사용하면 될 것 같아요! 스터디 때 좀 더 논의해봅시다!
| const RequiredFormItem = styled(FormItem)` | ||
| label::after { | ||
| padding-left: 4px; | ||
| color: var(--primary-color); | ||
| content: "*"; | ||
| } | ||
| `; |
There was a problem hiding this comment.
[논의]
필수 항목 표시(*)를 RequiredFormItem = styled(FormItem)으로 확장해서 처리한 방식 좋네요! 저는 같은 문제를 $required prop으로 분기 처리했는데, 비교해보니 트레이드오프가 있는 것 같아요.
styled(Component) 확장은 variant마다 이름 있는 컴포넌트가 생겨서 의도가 명확하다는 장점이 있는 반면, variant가 늘어나면 컴포넌트 개수도 같이 늘어날 것 같아요. 저처럼 props로 처리하면 컴포넌트 하나로 여러 variant를 다룰 수 있지만, 조건이 많아지면 템플릿 리터럴 안 분기가 복잡해지는 단점이 있더라고요.
지금 규모에서는 두 방식 다 괜찮은 것 같은데, variant가 더 늘어날 걸 가정하면 어느 쪽이 더 나을지 스터디 때 이야기해보면 좋을 것 같습니다!
| img { | ||
| display: block; | ||
| width: 40px; | ||
| height: 40px; | ||
| object-fit: contain; | ||
| } | ||
| `; |
There was a problem hiding this comment.
[배움]
저는 CSS 모듈에서도 아이콘에 별도 클래스를 지정해놨어서 styled-components로 옮길 때도 별도 컴포넌트로 분리했는데, 팀원 코드 보니 이 부분은 중첩이 더 간결한 선택일 수도 있겠네요.
| const List = styled.ul` | ||
| padding: 0 16px; | ||
| margin: 16px 0; | ||
| `; |
There was a problem hiding this comment.
[질문]
section+ul 이중 구조를 ul 하나로 단일화하신 점이 독특하네요! 저는 기존의 이중 구조를 유지했는데, section이 리스트 영역이라는 의미를 명시적으로 드러내준다고 생각해서였어요.
다만 지금처럼 section 안에 ul 하나만 있는 경우엔 굳이 감쌀 필요가 있나 싶기도 해서, 더 단순한 구조도 충분히 설득력 있어 보입니다! DOM 노드 하나 줄이는 것과 시맨틱 명시 사이에서 어떤 기준으로 선택하셨는지 궁금해요.
There was a problem hiding this comment.
<ul> 자체가 항목들의 목록이라는 의미를 가진 시맨틱 요소라서, 음식점 목록이라는 의도를 <ul>만으로 충분히 전달할 수 있다고 판단했습니다.
| --grey-50: #fcfcfd; | ||
| --grey-100: #ffffff; | ||
| --grey-150: #e9eaed; |
There was a problem hiding this comment.
[배움]
색상 하드코딩을 :root 변수로 옮기신 부분이 인상적이네요!
저는 미션 요구사항에서 App.css를 고정해놔서 그대로 styled-components 스타일로 변환하는 것에만 신경썼는데, 세심한 접근인 것 같아요.
처음에는 제약 사항을 어기는 것이 아닌가? 하는 생각이 들었는데, 이번 미션 제약의 타겟은 컴포넌트별 스타일 클래스가 App.css에 남아있는 것을 막는 것이지, 색상 변수를 추가하는 것 자체를 막으려는 의도는 아닐 가능성이 크다고 판단했습니다.
색상 변수는 컴포넌트 스타일이 아니라 디자인 토큰이기 때문에 styled-components 안에서도 var()로 그대로 참조되니, 토큰을 추가하는 건 미션 취지에 어긋나지 않는다고 봐서 저도 수정 반영했습니다!
meteorqz6
changed the title
2 participants
개인 목표 달성 여부
리뷰어에게
Summary by CodeRabbit
릴리스 노트
Documentation
Dependencies
Refactor