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

fix: sync form state with inputs (fix #1233) #1235

Merged
merged 2 commits into from
Aug 3, 2022

Conversation

adhrinae
Copy link
Contributor

@adhrinae adhrinae commented Aug 1, 2022

Please check if the PR fulfills these requirements

  • It's the right issue type on the title
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • It does not introduce a breaking change or has a description of the breaking change

Description

  • Synced form state with input components
    • Some of formState was irrelevant to the popup. So it was removed.
  • Reset form states when closing the popup

Resolves #1233

As is

Screen.Recording.2022-08-01.at.11.26.18.mov

To be

Screen.Recording.2022-08-01.at.11.26.48.mov

Thank you for your contribution to TOAST UI product. 🎉 😘 ✨

@adhrinae
Copy link
Contributor Author

adhrinae commented Aug 1, 2022

E2E 테스트 한 건이 실패할테지만 #1236 에서 해결될 예정

Copy link
Contributor

@lja1018 lja1018 left a comment

Choose a reason for hiding this comment

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

리뷰 완료합니다.

Copy link
Contributor

@jajugoguma jajugoguma left a comment

Choose a reason for hiding this comment

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

리뷰 완료합니다.

Copy link
Member

@heenakwag heenakwag left a comment

Choose a reason for hiding this comment

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

리뷰 완료합니다.

apps/calendar/src/components/popup/eventFormPopup.tsx Outdated Show resolved Hide resolved
calendarId: event?.calendarId ?? calendars[0]?.id,
state: eventState,
});
const [formState, formStateDispatch] = useFormState(calendars[0]?.id);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

애초에 useFormState 선언 시 기본값을 주는 것은 거의 의미가 없습니다. 컴포넌트 마운트 시 최초에 한 번만 실행되기 때문입니다. 그런데 그 때는 대부분의 값이 비어있겠죠.

@@ -155,20 +137,28 @@ export function EventFormPopup() {
}
}, [layoutContainer, popupArrowPointPosition]);

// Reset form states when closing the popup
// Sync store's popupParams with formState when editing event
Copy link
Contributor Author

Choose a reason for hiding this comment

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

팝업이 열릴 때, 이벤트를 수정하는 상태일 경우 event 값이 있습니다. 그래서 이 상태를 반영합니다.
팝업은 열리지만 이벤트를 생성하는 경우에는 calendarId를 제외하고 나머지 필드는 비어있어야 합니다.
다른 캘린더의 동작을 참고해보았을때, 이전에 어떤 캘린더를 선택한 상태에서 팝업을 닫고 다시 열었을 때는 직전에 선택한 캘린더가 유지되는 동작을 하고 있었습니다. 그래서 이벤트 생성시에도 직전에 선택된 calendarId 를 되도록이면 유지하려고 했습니다.

},
});
}
}, [calendars, event, formStateDispatch, popupParams]);

// Reset form states when closing the popup
Copy link
Contributor Author

Choose a reason for hiding this comment

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

formState를 제대로 활용하기 전에는 팝업을 닫자마자 input 태그의 값이 모두 날아가버렸기 때문에 이런 과정이 필요 없었습니다. 하지만 formState의 상태와 input 태그의 value를 연결했기 때문에 제어가 필요합니다.

>
) {
return useReducer(formStateReducer, initialState);
export function useFormState(initCalendarId?: string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

calendarId가 없으면 기본값을 '' 으로 하려 했는데 기존 테스트가 깨지는 것을 보고 다 같이 수정해줄까 하다 이대로 유지하기로 했습니다.

Copy link
Member

@heenakwag heenakwag left a comment

Choose a reason for hiding this comment

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

리뷰 완료합니다. 코드가 더 명확해지고 캘린더 id 유지되는 것도 좋네요:+1::+1: 꼼꼼히 신경쓰시느라 고생하셨습니다

Copy link
Contributor

@lja1018 lja1018 left a comment

Choose a reason for hiding this comment

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

리뷰 완료합니다.

Copy link
Contributor

@jajugoguma jajugoguma left a comment

Choose a reason for hiding this comment

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

리뷰 완료합니다! 고생하셨습니다!

@adhrinae adhrinae merged commit 5c7a012 into main Aug 3, 2022
@adhrinae adhrinae deleted the fix/form-popup-state-sync branch August 3, 2022 02:34
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.

Which source should I refer to see the allday toggle function?
4 participants