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

[Feature/BAR-37] 토스트 컴포넌트 개발 #16

Merged
merged 17 commits into from
Dec 30, 2023
Merged

Conversation

miro-ring
Copy link
Contributor

@miro-ring miro-ring commented Dec 24, 2023

Summary

구현 내용 및 작업한 내역을 요약해서 적어주세요

To Reviewers

애니메이션과 관련해서는 배포 이후에 디자인 기획쪽에 확인 요청 드릴 예정입니다.

097f55b
토스트 테스트를 위한 환경은 머지 직전에 제거할 에정입니다.

2줄 이상의 토스트는 디자인에 보이지 않아서 일단 대응하지 않았습니다.

How To Test

홈 페이지에서 "토스트 노출" 클릭시 토스트가 정상 노출되어야 합니다.

@miro-ring miro-ring self-assigned this Dec 24, 2023
@dmswl98 dmswl98 changed the title [Feature/Bar-37] 토스트 컴포넌트 개발 [Feature/BAR-37] 토스트 컴포넌트 개발 Dec 24, 2023
Copy link
Member

@wonjin-dev wonjin-dev left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 :)

토스트 2줄 대응은 저도 불필요하다고 생각해요 !!

.eslintrc.json Outdated
@@ -23,6 +23,7 @@
"noUselessIndex": true
}
],
"react-hooks/exhaustive-deps": "off",
Copy link
Member

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.

@wonjin-dev
요거 deps에 넣고 싶지 않은 내용까지 넣으라고 추천하고 있어서 빼긴 했습니다.
요 룰 필요할까요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wonjin-dev
70f54ca
요 부분 반영해보았습니다. 감사합니다 ㅎㅎ

},
}));

export const useToastStore = <T>(
Copy link
Member

Choose a reason for hiding this comment

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

useToast 등을 통해 한 번 더 래핑해서,
사용처에서 셀렉터가 아니라 const { showToast } = useToast();
요런 느낌이면 더 편할것 같은데, 어떻게 생각하시나용 ??

Copy link
Member

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.

@wonjin-dev @dmswl98
감사합니다 ㅎㅎ 해당 형태가 훨씬 좋겠네요!
634e3df
해당 커밋에 반영해보았습니다 ㅎㅎ

Copy link
Member

@dmswl98 dmswl98 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!

pages/index.tsx Outdated
Comment on lines 6 to 17
const showToast = useToastStore((state) => state.showToast);

return (
<div>
<button
type="button"
onClick={() => showToast({ message: '토스트 노출' })}
>
토스트 노출
</button>
</div>
);
Copy link
Member

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.

@dmswl98
그게 훨씬 좋겠네요 ㅎㅎ 감사합니다.
a8f5b62
해당 커밋에서 반영해보았습니다!

Comment on lines 32 to 28
return (
<div className={clsx(toast, isToastVisible && open)} role="alert">
<span>{message}</span>
</div>
);
Copy link
Member

Choose a reason for hiding this comment

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

clsx 대신, 이렇게 작성할 수도 있습니다.

const TabsItem = ({ children, path }: PropsWithChildren<TabsItemProps>) => {
  const pathname = usePathname();

  return (
    <a href={path} className={styles.tabItem({ isActive: path === pathname })}>
      {children}
    </a>
  );
};
export const tabItem = recipe({
  base: {
    fontSize: '16px',
    letterSpacing: '-0.5px',
    paddingBottom: '4px',
    borderBottom: '2px solid transparent',
  },
  variants: {
    isActive: {
      true: {
        fontWeight: 700,
        borderBottomColor: '#121212',
      },
    },
  },
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmswl98
3305844
오 감사합니다!! 해당 커밋에서 반영해보았습니다.
style import 할 때 어떤 형태로 할 지에 대한 룰도 있으면 좋지 않을까하는 생각이 들었습니다.

},
}));

export const useToastStore = <T>(
Copy link
Member

Choose a reason for hiding this comment

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

저도 동의해요!

Comment on lines +25 to +28
export const toastStore = createStore<State & Action>((set, get) => ({
toastData: INITIAL_TOAST_DATA,
Copy link
Member

Choose a reason for hiding this comment

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

create가 아니라 createStore는 어떤 걸까요..? 문서에서 찾지 못했어요ㅜㅜ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Base automatically changed from feature/BAR-49 to main December 27, 2023 18:59
Copy link

Copy link

@miro-ring
Copy link
Contributor Author

@wonjin-dev @dmswl98
2종류의 toast duration을 반영합니다.

lint-staged/lint-staged#825
lintstage 실행시 모듈을 찾지 못하는 문제가 있었고 bash 커맨드 추가를 통해 해결하였습니다.

Copy link
Member

@dmswl98 dmswl98 left a comment

Choose a reason for hiding this comment

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

늦게 확인했었네요ㅠㅠ 수고하셨습니다! 👍🏻 바로 머지 진행해주세요~

src/components/Toast/Toast.stories.tsx Outdated Show resolved Hide resolved
src/components/Toast/Toast.stories.tsx Outdated Show resolved Hide resolved
Copy link

Copy link

@miro-ring miro-ring merged commit 27ed97b into main Dec 30, 2023
2 of 3 checks passed
@miro-ring miro-ring deleted the feature/BAR-37 branch December 30, 2023 03:46
@dmswl98 dmswl98 added the feature label Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants