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

Dev/miyu #17

Merged
merged 21 commits into from
Mar 6, 2024
Merged

Dev/miyu #17

merged 21 commits into from
Mar 6, 2024

Conversation

Minkyu01
Copy link
Contributor

@Minkyu01 Minkyu01 commented Mar 6, 2024

이렇게 pr을 하는거였구나

Copy link

cloudflare-workers-and-pages bot commented Mar 6, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7cafc51
Status: ✅  Deploy successful!
Preview URL: https://af3e8bab.pick-of-pig.pages.dev
Branch Preview URL: https://dev-miyu.pick-of-pig.pages.dev

View logs

src/app/page.tsx Outdated

function Home() {
console.log(score); // undeFined
Copy link
Collaborator

Choose a reason for hiding this comment

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

필요없는 코드인 것 같습니다. 지우는게 좋을 것 같네요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

function HeaderNav() {
const categoryLists = useRecoilValue(categoryList);
const [searchAddress, setSearchAddress] = useState('');
// 전역변수로 만들고 싶네
Copy link
Collaborator

@LuticaCANARD LuticaCANARD Mar 6, 2024

Choose a reason for hiding this comment

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

전역변수로 하고 싶은 이유를 혹시 알 수 있을까요? 상태 관리가 복잡해질 수는 있겠지만 필요하다면 도입해도 괜찮지않을까 싶습니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SearchBar가 map의 상단과 모달에서도 있어서 컴포넌트화 시킬려고 해서 recoil로 하고 싶다는 거였는데, map디자인 확정되고, 코드 구조좀 바뀔때 다시 한번 볼게...


const openModal = (name: string) => {
{
name === 'LeftNav' ? setIsModalOpen(true) : setIsRandomModal(true);
Copy link
Collaborator

@LuticaCANARD LuticaCANARD Mar 6, 2024

Choose a reason for hiding this comment

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

 setIsModalOpen(name === 'LeftNav' ) 

코드가 더 간결해보이네요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

잠깐 딴거하다 다시봤는데

이경우에 eslint가 Expected an assignment or function call and instead saw an expression. eslint@typescript-eslint/no-unused-expressions를 띄웁니다.

제 생각에, 함수를 실행하는 경우의 조건분기라면,

    if (name === 'LeftNav') {
      setIsModalOpen(true);
    } else {
      setIsRandomModal(true);
    }

와 같이 if문을 쓰는게 좋아보입니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오 알겠어!!

document.addEventListener('mousedown', handleClickOutside);
// 클린업 함수 ,,? 모르겠음
return () => {
document.removeEventListener('mousedown', handleClickOutside);
Copy link
Collaborator

@LuticaCANARD LuticaCANARD Mar 6, 2024

Choose a reason for hiding this comment

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

이 컴포넌트가 지워진 다음, remove를 해줘야 다시 실행되는 일이 없겠죠?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

근데 내가 주석처리하고 해봐도 큰 차이가 없었어!!!

<SearchComponentContainer id="searchComponent">
{optionLists.map((it, index) => (
<SearchComponentStyled key={index}>
<div>{it}</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Array의 index는 key로 쓰기에 부적합해요!

  • Do not use Array index in keyseslintreact/no-array-index-key
  • Dom 이 rendering될때, 객체의 순서가 바뀌거나 객체값이 변하면, 데이터를 다루는 컴포넌트도 달라져요.
  • 카테고리 자체가 유일하니 카테고리를 key로 쓰는걸로 바꿀게요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그럼 index대신에 it 넣으라고 하는거지?

return (
<ListModalStyled>
<ModalContentStyled ref={modalRef}>
<SearchBar />
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 컴포넌트에는 필요한 값이 있는 것 같은데, 왜 없는지 혹시 알 수 있을까요?

'{}' 형식에 '{ handleInputChange: (e: ChangeEvent<HTMLInputElement>) => void; handleSearchClick: () => void; searchAddress: string; }' 형식의 handleInputChange, handleSearchClick, searchAddress 속성이 없습니다.ts(2739)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그거 아직 디자인 fix가 안되어서 SearchBar를 어느 단에서 불러올지 확실하지 않아서 일단 남겼어, 위에 보면 전역변수화 시키고 싶다는 부분이 이거때문이야

Copy link
Collaborator

@LuticaCANARD LuticaCANARD left a comment

Choose a reason for hiding this comment

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

LGTM

@LuticaCANARD LuticaCANARD merged commit 4043359 into main Mar 6, 2024
1 check passed
@Minkyu01 Minkyu01 deleted the dev/miyu branch May 15, 2024 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants