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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 78 additions & 1 deletion apps/calendar/src/components/popup/eventFormPopup.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { EventFormPopup } from '@src/components/popup/eventFormPopup';
import { initCalendarStore, useDispatch } from '@src/contexts/calendarStore';
import { useEventBus } from '@src/contexts/eventBus';
import { cls } from '@src/helpers/css';
import { fireEvent, render, screen } from '@src/test/utils';
import { fireEvent, render, screen, userEvent } from '@src/test/utils';
import TZDate from '@src/time/date';

import type { Options } from '@t/options';
Expand Down Expand Up @@ -187,4 +187,81 @@ describe('event form popup', () => {
location: changedLocation,
});
});

// Regression tests for #1233
it('should preserve input values when "All day" checkbox is toggled', async () => {
// Given
setup();
const user = userEvent.setup();
const getTitleInput = (): HTMLInputElement => screen.getByPlaceholderText('Subject');
const getLocationInput = (): HTMLInputElement => screen.getByPlaceholderText('Location');
const allDayCheckbox = screen.getByText('All day');

const givenTitle = 'title';
const givenLocation = 'location';

await user.type(getTitleInput(), givenTitle);
await user.type(getLocationInput(), givenLocation);

// When
await user.click(allDayCheckbox);

// Then
expect(getTitleInput().value).toBe(givenTitle);
expect(getLocationInput().value).toBe(givenLocation);

// When change input and toggle again
const concatStr = ' changed';
await user.type(getTitleInput(), concatStr);
await user.type(getLocationInput(), concatStr);
await user.click(allDayCheckbox);

// Then
expect(getTitleInput().value).toBe(`${givenTitle}${concatStr}`);
expect(getLocationInput().value).toBe(`${givenLocation}${concatStr}`);
});

it('should preserve input values when selecting calendar', async () => {
// Given
const calendars = [
{
id: '1',
name: 'Personal',
},
{
id: '2',
name: 'Work',
},
];
setup(calendars);

const user = userEvent.setup();
const getTitleInput = (): HTMLInputElement => screen.getByPlaceholderText('Subject');
const getLocationInput = (): HTMLInputElement => screen.getByPlaceholderText('Location');

const givenTitle = 'title';
const givenLocation = 'location';

await user.type(getTitleInput(), givenTitle);
await user.type(getLocationInput(), givenLocation);

// When
await user.click(screen.getByRole('button', { name: calendars[0].name }));
await user.click(screen.getByText(calendars[1].name));

// Then
expect(getTitleInput().value).toBe(givenTitle);
expect(getLocationInput().value).toBe(givenLocation);

// When change input and toggle again
const concatStr = ' changed';
await user.type(getTitleInput(), concatStr);
await user.type(getLocationInput(), concatStr);
await user.click(screen.getByRole('button', { name: calendars[1].name }));
await user.click(screen.getByText(calendars[0].name));

// Then
expect(getTitleInput().value).toBe(`${givenTitle}${concatStr}`);
expect(getLocationInput().value).toBe(`${givenLocation}${concatStr}`);
});
});
60 changes: 32 additions & 28 deletions apps/calendar/src/components/popup/eventFormPopup.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { h } from 'preact';
import { createPortal } from 'preact/compat';
import { useLayoutEffect, useMemo, useRef, useState } from 'preact/hooks';
import { useEffect, useLayoutEffect, useMemo, useRef, useState } from 'preact/hooks';

import type { DateRangePicker } from 'tui-date-picker';

Expand All @@ -24,13 +24,13 @@ import { useFloatingLayer } from '@src/contexts/floatingLayer';
import { useLayoutContainer } from '@src/contexts/layoutContainer';
import { cls } from '@src/helpers/css';
import { isLeftOutOfLayout, isTopOutOfLayout } from '@src/helpers/popup';
import { useFormState } from '@src/hooks/popup/useFormState';
import { FormStateActionType, useFormState } from '@src/hooks/popup/useFormState';
import type EventModel from '@src/model/eventModel';
import { calendarSelector } from '@src/selectors';
import { eventFormPopupParamSelector } from '@src/selectors/popup';
import TZDate from '@src/time/date';
import { compare } from '@src/time/datetime';
import { isNil } from '@src/utils/type';
import { isNil, isPresent } from '@src/utils/type';

import type { FormEvent, StyleProp } from '@t/components/common';
import type { BooleanKeyOfEventObject, EventObject } from '@t/events';
Expand Down Expand Up @@ -97,31 +97,11 @@ export function EventFormPopup() {
const { calendars } = useStore(calendarSelector);
const { hideAllPopup } = useDispatch('popup');
const popupParams = useStore(eventFormPopupParamSelector);
const {
title,
location,
start,
end,
isAllday = false,
isPrivate = false,
eventState = 'Busy',
popupArrowPointPosition,
close,
isCreationPopup,
event,
} = popupParams ?? {};
const { start, end, popupArrowPointPosition, close, isCreationPopup, event } = popupParams ?? {};
const eventBus = useEventBus();
const formPopupSlot = useFloatingLayer('formPopupSlot');
const [formState, formStateDispatch] = useFormState({
title,
location,
start,
end,
isAllday,
isPrivate,
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 선언 시 기본값을 주는 것은 거의 의미가 없습니다. 컴포넌트 마운트 시 최초에 한 번만 실행되기 때문입니다. 그런데 그 때는 대부분의 값이 비어있겠죠.


const datePickerRef = useRef<DateRangePicker>(null);
const popupContainerRef = useRef<HTMLDivElement>(null);
const [style, setStyle] = useState<StyleProp>({});
Expand Down Expand Up @@ -157,6 +137,30 @@ export function EventFormPopup() {
}
}, [layoutContainer, popupArrowPointPosition]);

// 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 를 되도록이면 유지하려고 했습니다.

useEffect(() => {
if (isPresent(popupParams) && isPresent(event)) {
formStateDispatch({
type: FormStateActionType.init,
event: {
title: popupParams.title,
location: popupParams.location,
isAllday: popupParams.isAllday,
isPrivate: popupParams.isPrivate,
calendarId: event.calendarId,
state: popupParams.eventState,
},
});
}
}, [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를 연결했기 때문에 제어가 필요합니다.

useEffect(() => {
if (isNil(popupParams)) {
formStateDispatch({ type: FormStateActionType.reset });
}
}, [formStateDispatch, popupParams]);

if (isNil(start) || isNil(end) || isNil(formPopupSlot)) {
return null;
}
Expand Down Expand Up @@ -198,11 +202,11 @@ export function EventFormPopup() {
<PopupSection />
)}
<TitleInputBox
title={title}
title={formState.title}
isPrivate={formState.isPrivate}
formStateDispatch={formStateDispatch}
/>
<LocationInputBox location={location} />
<LocationInputBox location={formState.location} formStateDispatch={formStateDispatch} />
<DateSelector
start={start}
end={end}
Expand Down
13 changes: 11 additions & 2 deletions apps/calendar/src/components/popup/locationInputBox.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import { h } from 'preact';
import type { ChangeEventHandler } from 'preact/compat';

import { PopupSection } from '@src/components/popup/popupSection';
import { cls } from '@src/helpers/css';
import type { FormStateDispatcher } from '@src/hooks/popup/useFormState';
import { FormStateActionType } from '@src/hooks/popup/useFormState';
import { useStringOnlyTemplate } from '@src/hooks/template/useStringOnlyTemplate';

interface Props {
location: string;
location?: string;
formStateDispatch: FormStateDispatcher;
}

const classNames = {
Expand All @@ -14,12 +18,16 @@ const classNames = {
content: cls('content'),
};

export function LocationInputBox({ location }: Props) {
export function LocationInputBox({ location, formStateDispatch }: Props) {
const locationPlaceholder = useStringOnlyTemplate({
template: 'locationPlaceholder',
defaultValue: 'Location',
});

const handleLocationChange: ChangeEventHandler<HTMLInputElement> = (e) => {
formStateDispatch({ type: FormStateActionType.setLocation, location: e.currentTarget.value });
};

return (
<PopupSection>
<div className={classNames.popupSectionItem}>
Expand All @@ -29,6 +37,7 @@ export function LocationInputBox({ location }: Props) {
className={classNames.content}
placeholder={locationPlaceholder}
value={location}
onChange={handleLocationChange}
/>
</div>
</PopupSection>
Expand Down
8 changes: 7 additions & 1 deletion apps/calendar/src/components/popup/titleInputBox.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { h } from 'preact';
import type { ChangeEventHandler } from 'preact/compat';

import { PopupSection } from '@src/components/popup/popupSection';
import { cls } from '@src/helpers/css';
Expand All @@ -7,7 +8,7 @@ import { FormStateActionType } from '@src/hooks/popup/useFormState';
import { useStringOnlyTemplate } from '@src/hooks/template/useStringOnlyTemplate';

interface Props {
title: string;
title?: string;
isPrivate?: boolean;
formStateDispatch: FormStateDispatcher;
}
Expand All @@ -28,6 +29,10 @@ export function TitleInputBox({ title, isPrivate = false, formStateDispatch }: P
const togglePrivate = () =>
formStateDispatch({ type: FormStateActionType.setPrivate, isPrivate: !isPrivate });

const handleInputChange: ChangeEventHandler<HTMLInputElement> = (e) => {
formStateDispatch({ type: FormStateActionType.setTitle, title: e.currentTarget.value });
};

return (
<PopupSection>
<div className={classNames.popupSectionItem}>
Expand All @@ -37,6 +42,7 @@ export function TitleInputBox({ title, isPrivate = false, formStateDispatch }: P
className={classNames.content}
placeholder={titlePlaceholder}
value={title}
onChange={handleInputChange}
required
/>
</div>
Expand Down
31 changes: 28 additions & 3 deletions apps/calendar/src/hooks/popup/useFormState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,36 +3,61 @@ import { useReducer } from 'preact/hooks';
import type { EventObject, EventState } from '@t/events';

export enum FormStateActionType {
init = 'init',
setCalendarId = 'setCalendarId',
setTitle = 'setTitle',
setLocation = 'setLocation',
setPrivate = 'setPrivate',
setAllday = 'setAllday',
setState = 'setState',
reset = 'reset',
}

type FormStateAction =
| { type: FormStateActionType.init; event: EventObject }
| { type: FormStateActionType.setCalendarId; calendarId: string }
| { type: FormStateActionType.setTitle; title: string }
| { type: FormStateActionType.setLocation; location: string }
| { type: FormStateActionType.setPrivate; isPrivate: boolean }
| { type: FormStateActionType.setAllday; isAllday: boolean }
| { type: FormStateActionType.setState; state: EventState };
| { type: FormStateActionType.setState; state: EventState }
| { type: FormStateActionType.reset };

export type FormStateDispatcher = (action: FormStateAction) => void;

const defaultFormState: EventObject = {
title: '',
location: '',
isAllday: false,
isPrivate: false,
state: 'Busy',
};

// eslint-disable-next-line complexity
function formStateReducer(state: EventObject, action: FormStateAction): EventObject {
switch (action.type) {
case FormStateActionType.init:
return { ...defaultFormState, ...action.event };
case FormStateActionType.setCalendarId:
return { ...state, calendarId: action.calendarId };
case FormStateActionType.setTitle:
return { ...state, title: action.title };
case FormStateActionType.setLocation:
return { ...state, location: action.location };
case FormStateActionType.setPrivate:
return { ...state, isPrivate: action.isPrivate };
case FormStateActionType.setAllday:
return { ...state, isAllday: action.isAllday };
case FormStateActionType.setState:
return { ...state, state: action.state };
case FormStateActionType.reset:
return { ...state, ...defaultFormState };

default:
return state;
}
}

export function useFormState(initialState: EventObject) {
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가 없으면 기본값을 '' 으로 하려 했는데 기존 테스트가 깨지는 것을 보고 다 같이 수정해줄까 하다 이대로 유지하기로 했습니다.

return useReducer(formStateReducer, { calendarId: initCalendarId, ...defaultFormState });
}