-
Notifications
You must be signed in to change notification settings - Fork 4
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/#294] 우선순위 입력 페이지에서 가능시간 입력 페이지로 되돌아가면 우선순위 초기화 #295
Changes from 7 commits
887f8e9
b83bb5e
453786a
3288a6d
9c0ba8f
6a94a11
6e833aa
00fc5d5
badc10e
e74fe5e
c5f1297
a4afb65
82fdfc8
b70d477
59b11d4
da1d46d
5b2463d
662ddba
5ee72ab
ed9af49
4307dc7
c7d3c94
69d9f7b
e7c38c3
fa61c71
3f7f222
b042fec
35a654d
927697a
b5064a9
7db8a09
44887d0
9b704a0
cd73519
60a6245
2d9b3c8
7b154d0
e70cdf9
71da64f
ff52666
5f5824f
34c2f0e
06968a9
0278b8e
26288e2
b760a34
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import { useState } from 'react'; | ||
import { useEffect, useState } from 'react'; | ||
|
||
import Timetable from 'components/timetableComponents/Timetable'; | ||
import { ColumnStructure, TimetableStructure } from 'components/timetableComponents/types'; | ||
|
@@ -8,6 +8,7 @@ import PriorityCta from './selectPriority/PriorityCta'; | |
import PriorityDropdown from './selectPriority/PriorityDropdown'; | ||
import SelectionColumn from './selectTimeSlot/SelectionColumn'; | ||
import TimeSlotCta from './selectTimeSlot/TimeSlotCta'; | ||
|
||
import { useScheduleStepContext } from '../contexts/useScheduleStepContext'; | ||
import { SelectContext, SelectedSlotType } from '../contexts/useSelectContext'; | ||
import { StepSlotsType, StepbottomItemsType } from '../types'; | ||
|
@@ -39,6 +40,23 @@ function SelectScheduleTable({ timeSlots, availableDates }: TimetableStructure) | |
}; | ||
const bottomItem = bottomItems[scheduleStep]; | ||
|
||
useEffect( | ||
() => { | ||
if (scheduleStep === 'selectTimeSlot') { | ||
const resetPriorities = () => { | ||
const updatedSchedules = Object.entries(selectedSlots).map((schedule) => { | ||
if (schedule[1] && typeof schedule[1] === 'object') { | ||
return [schedule[0], { ...schedule[1], priority: 0 }]; | ||
} | ||
return schedule; | ||
}); | ||
setSelectedSlots(Object.fromEntries(updatedSchedules)); | ||
}; | ||
resetPriorities(); | ||
} | ||
}, | ||
[scheduleStep], | ||
); | ||
return ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분 회의때도 얘기했지만 다시 정리해서 의견 남기겠습니다!
react developer tools로 직접 확인해보니 역시 렌더링이 두번 일어나고 있었습니다. 2024-08-04.3.31.32.movuseEffect 사용 O(초기화 기능 O) 2024-08-04.3.31.56.mov제 생각에 selectedSlots라는 하나의 state에서, (가능 시간 입력 페이지에서 사용되는) 선택된 날짜 데이터와 (우선순위 입력 페이지에서 사용되는) priority 데이터가 같이 관리되고 있다는 구조적인 문제에서 원인을 따져볼 수도 있을 것 같습니다. 가능 시간 입력 시간표와 우선순위 입력 시간표가 아예 다른 컴포넌트로 분리되어있다면, 가능 시간 입력 시간표에서는 priority값을 사용할 필요가 없고, 우선순위 입력 시간표가 렌더링 될 때 priority가 항상 0으로 초기화되게끔 한다면 해결이 될 수도 있지 않을까....
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 추가로, entries메소드 사용으로 인한 번거로움을 언급해주셨는데 다음과 같이 작성하면 어떨까요? const resetPriorities = (selectedSlots: SelectedSlotType) => {
const updatedSelectedSlots: SelectedSlotType = {};
for (const key in selectedSlots) {
updatedSelectedSlots[key] = {
...selectedSlots[key],
priority: 0,
};
}
setSelectedSlots(updatedSelectedSlots);
};
useEffect(
() => {
if (scheduleStep === 'selectTimeSlot') resetPriorities(selectedSlots);
},
[scheduleStep],
); 추가로, useEffect 내부에서 실행할 작업이 명시적으로 드러나는 게 좋다고 생각해서 위와 같이 resetPriorities 함수를 따로 빼는것이 어떨까합니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 말씀해주신 모든 의견이 좋아보입니다. 배열로 변경하지 않고도 충분히 set할수 있겠군요!
이런 로직을 첫 렌더링때 사용하려면 결국 useEffect가 필요할 것 같군요.. header에 onClick함수를 전달하는 방향으로 header가 개선되는 방향이 가장 적합할 것 같다는 생각이 드는데 어떻게 생각하시나요?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. useEffect 사용하지 않고 현재 상황에서 해결할 수 있는 방안을 찾았습니다. 저 역시 effect를 사용하는 것에 충분히 이질감을 느끼고 있었고 위와 같은 생각을 하며 추후 개선해야지.. 하고있다가 조금 더 고민해보면서 effect를 사용하지 않고 평소의 흐름에서 state를 초기화할 수 있는 경우의 수를 생각해봤어요
그래서 저희는 일반적인 렌더링 흐름에 맞게 2번을 선택할 수 있을 것 같아요! 아래코드 참고해주시고 의견주시면 감사하겠습니다! 또는 제가 놓치는 부분 있으면 편히 말씀해주세요!
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 왓!! 뭔가 방법이 있을 것 같다고 생각했는데, 다음 버튼에 로직을 심는 접근 아주 좋은 것 같습니다!!! 너무 좋은 솔루션이네요 창의력 👍 |
||
<SelectContext.Provider | ||
value={{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
z-index를 계속 하드코딩 하게 되면 앞으로도 모달, 바텀시트 등등 z-index가 중요한 상황에서 예상치 못한 결과가 생길 수 있으니
사이드바의 z-index를 늘리는 것보다, swiper에 기본적으로 적용되어 있는 z-index: 1을 없애는게 낫지 않을까?라는 생각이 들었어요.
그래서 swiper에 왜 z-index: 1이 적용되어 있는지를 좀 알아봤는데, 해당 swiper 라이브러리 코드를 보니 z-index 부분에 대해
Fix of Webkit flickering
이라는 주석이 써있더라구요. 관련 PR이 없어서 더 자세히 알아보지는 못했어요.일단 제가 swiper에서 z-index를 0으로 맞추고 크롬과 사파리에서 모두 확인해보았으나 문제는 없었는데... swiper 개발자가 z-index를 넣은데는 이유가 있을테니까요..ㅎㅎ
그래서 결론은, 우선 이렇게 해두되 이번 스프린트 끝내고나서 z-index를 상수로 빼서 관리를 하는 등 위계를 잘 정리해보면 좋을 것 같습니다.
서비스 사이즈가 작다보니 z-index가 쓰이는 곳도 그리 많지 않은 것 같아서요. 헤더, 사이드바, 드롭다운, 모달, 바텀시트 등..? 각각에 맞게 z-index를 상수화하면 덜 복잡하게 관리할 수 있을 것 같습니다.
결론만 확인해주시면 될거같긴한데 제 생각 과정을 공유드려봤읍니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋네요 z-index를 상수화한다는 생각은 못해봤는데 공부를 해봐야겠네요 !
이런식의 상수화를 말씀하시는게 맞으신가요??
완전 좋지만 그전에 필요없는 z-index사용한 부분을 개선해보면 좋겠네요!!
swiper의 z-index:1이 적용되어있는 이유는 Webkit의 깜빡임 문제때문이라고 하네요. webkit은 마크업을 기반으로 웹브라우저를 만드는데 기반을 제공하는 오픈소스 프레임워크라고 합니다. 저도 찾아보며 공부할 수 있어서 좋았습니다!
갑자기 궁금해서 z-index에 대해서도 조금 더 공부해보니
라고 이해하고 사용중입니다.
감사합니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 첨부해주신 링크에 잘 설명되어있네요!! 제가 처음에 생각한 방법은 아래처럼 단순한 상수화였는데, 어떤 방법이 적절할지는 저희 서비스내에 zIndex가 필요한 부분을 정리해보고 결정하면 좋을 것같아요. zIndex가 필요하지 않은데 쓰이고있는 것들이 있다면 정리하구요.