Skip to content

Comments

✨ feat : new mypage notice#50

Open
GuKBABjOa wants to merge 25 commits intomainfrom
feature/New-Mypage-Notice
Open

✨ feat : new mypage notice#50
GuKBABjOa wants to merge 25 commits intomainfrom
feature/New-Mypage-Notice

Conversation

@GuKBABjOa
Copy link
Contributor

@GuKBABjOa GuKBABjOa commented Jan 9, 2025

📍 PR 타입 (하나 이상 선택)

  • 기능 추가
  • 버그 수정
  • 의존성, 환경 변수, 빌드 관련 코드 업데이트
  • 기타 사소한 수정

❗️ 관련 이슈 [#]

📄 개요

mypage에서 스크랩한 공고를 클릭하면 보이는 화면을 구현하였습니다

🔁 변경 사항

변경 내용을 적어주세요 (커밋 번호를 적어주세요)

ScrapNotice.tsx 생성, dropdown.tsx 변경, searchbar.tsx변경

📸 스크린샷

image

image

image

👀 기타 더 이야기해볼 점

@GuKBABjOa GuKBABjOa self-assigned this Jan 9, 2025
@GuKBABjOa GuKBABjOa added the ✨ feat 기능 개발 label Jan 9, 2025
@GuKBABjOa GuKBABjOa added this to the 마이페이지 milestone Jan 9, 2025
@GuKBABjOa GuKBABjOa linked an issue Jan 9, 2025 that may be closed by this pull request
1 task
Copy link
Contributor

@junhakjh junhakjh left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!! 리뷰 읽고 천천히 반영 또는 답변 부탁드려요 :)

import SearchBar from '../ui/searchbar/SearchBar';
import { MRecruitDetail } from '@/mocks/data/recruit';

const getStatus = (startDate: Date, endDate: Date): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 함수는 ScrapNotice 내부로 들어가면 코드 이해에 좀 더 도움이 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

좋은 의견입니다! 수정했습니다.

const ScrapNotice: React.FC = () => {
const [selectedStatus, setSelectedStatus] = useState<string>('모집중');

interface Recruit {
Copy link
Contributor

Choose a reason for hiding this comment

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

타입명은 T~~~로 하기로 했어서 컨벤션 맞춰주시면 감사하겠습니다!
그리고 props를 제외한 모든 타입은 model 폴더에서 관리하기로 해서 적절히 이동시켜 주면 좋을 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TRecruitNotice를 model에 추가해 사용하는걸로 변경했습니다.

<table className='w-full border-collapse rounded-sm border border-gray-300'>
<thead>
<tr className='bg-gray-100'>
<th className='border border-gray-300 px-4 py-2'>공고명</th>
Copy link
Contributor

Choose a reason for hiding this comment

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

전체적으로 border border-gray-300 px-4 py-2가 중복되고 있는 것 같아요. 따로 TABLE_CELL_CLASSNAME 등의 적절한 전역변수를 컴포넌트 밖에 선언하고 사용하면 더 좋을 것 같아요!
참고로, 모든 요소에 border를 적용하는 것보다는 divide-x, divide-y 등의 클래스명을 활용해 봐도 괜찮을 것 같습니다 :)
TailwindCSS-Divide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오 이거 상당히 좋네요. 감사합니다.

<button
id={id}
className='flex w-full items-center gap-2 rounded-sm border border-input px-4 py-2.5 text-body2 focus:outline-activate'
className={className ? className : defaultClassName}
Copy link
Contributor

Choose a reason for hiding this comment

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

이러한 방식으로 코드를 작성하면 className을 props로 넘기는 경우 기존의 스타일이 모두 사라진다는 단점이 생길 것 같아요.
className={ className + " " + defaultClassName }으로 바꾸는 것은 어떨지 제안해봅니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

제가 조사를 해본 결과
className = '크기5 크기10' 이되면 tailwind에서는 크기10을 적용한다고 해서 직접해보았습니다
py-2.5와 py-0.5를 사용해보니
py-2.5 py-0.5, py-0.5 py-2.5둘다 py2.5가 적용되었고 결론적으로 className에 중복된 요소가 들어가면 안된다는 사실을 발견했습니다.
준하님의 의견을 받아들여
className={${defaultClassName} ${className || ""}}
이렇게 변경하려고 하였으나 적용이 제대로 되지않아 원상태로 유지하는 것이 나아보입니다.

placeholder='기술 스택 검색'
className='w-full rounded-full border border-blue-400 px-4 py-2 text-sm focus:outline-none focus:ring-2 focus:ring-blue-400'
placeholder={placeholder}
className={`w-full ${className
Copy link
Contributor

Choose a reason for hiding this comment

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

현재 컴포넌트의 12번째 라인과 형식을 통일하는 것이 좋을 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

변경 했습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

✨ feat 기능 개발

Projects

None yet

Development

Successfully merging this pull request may close these issues.

✨ feat: mypage-notice

3 participants