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.

Sign up for GitHub

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

Jump to bottom

fix: 이것저것 수정 #47

Merged
merged 16 commits into from
Aug 22, 2024
Merged

fix: 이것저것 수정 #47

merged 16 commits into from
Aug 22, 2024

Conversation

haryung-lee
Copy link
Member

@haryung-lee haryung-lee commented Aug 21, 2024 •
edited
Loading

작업 내용

  • 이것저것 수정하였습니당 (커밋 기록에 남겨두었슴당)

체크리스트

  • Code Review 요청
  • Label 설정

@haryung-lee haryung-lee added the 👻 Fix 수정 label Aug 21, 2024
@haryung-lee haryung-lee self-assigned this Aug 21, 2024
@haryung-lee haryung-lee requested a review from young-do as a code owner August 21, 2024 16:46
young-do
young-do reviewed Aug 22, 2024
View reviewed changes
Copy link
Collaborator

@young-do young-do left a comment

Choose a reason for hiding this comment

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

넘 챙겨줘서 고맙고맙
근데 궁금한점이 좀 있어서 코멘트 남김!

src/renderer/pages/naming.tsx Outdated
@@ -89,7 +89,8 @@ const Naming = () => {
const specialCharRegex = /[~!@#$%^&*()_+|<>?:{}\s]/;

const getErrorMessage = (name: string) => {
if (name.length === 0) return '';
if (name.length === 0) return ' ';
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
Member Author

@haryung-lee haryung-lee Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

지금은 name이 빈 스트링이여도 바뀌어지는데 그거 막으려구!
버튼 disabled 조건이 에러메세지 여부여서 내용 없는 빈칸을 줬엉

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
Member Author

Choose a reason for hiding this comment

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

아~~~~~~~~ 버튼 disabled 처리 안해줘도 되는구나!! ㅋㅎㅋㅎ 잘못봤다 요건 다시 수정할게~

src/renderer/pages/naming.tsx Outdated
@@ -89,7 +89,8 @@ const Naming = () => {
const specialCharRegex = /[~!@#$%^&*()_+|<>?:{}\s]/;

const getErrorMessage = (name: string) => {
if (name.length === 0) return '';
if (name.length === 0) return ' ';
if (name === ' ') return '고양이 이름은 빈 칸이 될 수 없어요';
Copy link
Collaborator

Choose a reason for hiding this comment

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

첫글자를 공백으로 입력하는 경우라니깐 startWith 를 쓰는게 좋지 않을까?

Copy link
Member Author

Choose a reason for hiding this comment

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

처음에 그걸 썼는데
에러메세지는 그냥 공백칸을 의미하는것 같아서!

그리구 startsWith 을 쓰면 공백+문자 일때도 "빈 칸이 될 수 없다" 로 나오는게 어색한듯..!

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
Member Author

Choose a reason for hiding this comment

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

그럼 공백 두개면 이 에러메시지가 보이는건가?

이 에러메세지는 안보일듯..!

아니면 에러문구를 수정하던가 '고양이 이름은 빈 칸이 될 수 없어요' 이건 뺄까?? 먼가.. 좀 애매하구먼

Copy link
Collaborator

Choose a reason for hiding this comment

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

#48 요걸 한번 봐주십셔 (_ _ )

Copy link
Member Author

Choose a reason for hiding this comment

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

코멘트들 반영하였습니다~~~!!

src/renderer/pages/onboarding.tsx
Comment on lines -79 to -81
<CarouselPrevious />
<CarouselNext />

Copy link
Collaborator

Choose a reason for hiding this comment

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

땡큐!

src/renderer/widgets/pomodoro/ui/home-screen.tsx Outdated
const body =
clickedMode === 'focus'
? { focusTime: createIsoDuration({ minutes }) }
: { restTime: createIsoDuration({ minutes }) };
updateCategory({
await updateCategory({
Copy link
Collaborator

Choose a reason for hiding this comment

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

updateCategory 내부를 보면 낙관적 업데이트를 구현해놔서
await 할 필요가 없음요!

Copy link
Member Author

Choose a reason for hiding this comment

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

아항,, 고러면 우선 async await 지우고 항상 토스트 띄워지게 할까??

Copy link
Collaborator

Choose a reason for hiding this comment

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

얍얍~

src/renderer/widgets/pomodoro/ui/home-screen.tsx Outdated
const body =
clickedMode === 'focus'
? { focusTime: createIsoDuration({ minutes }) }
: { restTime: createIsoDuration({ minutes }) };
updateCategory({
await updateCategory({
no: categories?.find((_category) => _category.title === category)?.no ?? 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

no: ... ?? 0 <- 0으로 전달되지 않게 하는게 더 좋을듯!

src/renderer/shared/ui/use-toast.ts Outdated
import type { ToastProps } from './toast';

const TOAST_LIMIT = 1;
const TOAST_REMOVE_DELAY = 1000000;

type ToasterToast = ToastProps & {
id: string;
iconName?: IconName;
icon: ReactNode;
Copy link
Collaborator

Choose a reason for hiding this comment

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

오? 왜 이렇게 바꾼걸까나..?

만약 icon classname 가 필요했다면 iconClassname 필드를 추가해서
넘겨주는건 어뗘?

Copy link
Member Author

Choose a reason for hiding this comment

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

icon 스타일때문이긴해..! 나는 아이콘 직접 넣어주는게 좀 더 편할것 같았는데
기존 방식대로 해볼게~~

src/renderer/widgets/pomodoro/ui/home-screen.tsx Outdated
setCurrentCategory(category);
toast({
icon: <Icon name="check" size="sm" className="[&>path]:stroke-icon-tertiary" />,
Copy link
Collaborator

Choose a reason for hiding this comment

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

아이콘 사이즈는 sm이 아니라 md 인듯!?

src/renderer/pages/mypage.tsx Outdated
@@ -2,9 +2,13 @@ import { useNavigate } from 'react-router-dom';

import { useFocusNotification } from '@/features/time';
import { useUser } from '@/features/user';
import OfflineStatIcon from '@/shared/assets/svgs/offline-stat.svg';
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
Member Author

Choose a reason for hiding this comment

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

진짜 전~~~~~~~~~혀 알아차리지 못했다 (너무 대충봤나봐 ㅎㅋㅎ...)

@young-do young-do mentioned this pull request Aug 22, 2024
Closed
@haryung-lee haryung-lee requested a review from young-do August 22, 2024 15:18
young-do
young-do approved these changes Aug 22, 2024
View reviewed changes
Copy link
Collaborator

@young-do young-do left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

@haryung-lee haryung-lee merged commit c8ac7d7 into main Aug 22, 2024
@haryung-lee haryung-lee deleted the fix/something branch August 22, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@young-do young-do young-do approved these changes

Assignees

@haryung-lee haryung-lee

Labels
👻 Fix 수정
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@haryung-lee @young-do