Timeline calendar num of days issues #2080
Conversation
src/expandableCalendar/index.tsx
Outdated
| @@ -174,12 +174,17 @@ const ExpandableCalendar = (props: ExpandableCalendarProps) => { | |||
| return CLOSED_HEIGHT + (WEEK_HEIGHT * (numberOfWeeks.current - 1)) + (hideKnob ? 12 : KNOB_CONTAINER_HEIGHT) + (constants.isAndroid ? 3 : 0); | |||
| }; | |||
| const openHeight = useRef(getOpenHeight()); | |||
| const closedHeight = useRef(CLOSED_HEIGHT + (hideKnob || Number(numberOfDays) > 1 ? 0 : KNOB_CONTAINER_HEIGHT)); | |||
| const closedHeight = useMemo(() => CLOSED_HEIGHT + (hideKnob || Number(numberOfDays) > 1 ? 0 : KNOB_CONTAINER_HEIGHT), [numberOfDays]); | |||
There was a problem hiding this comment.
missing dep 'hideKnob'
src/expandableCalendar/index.tsx
Outdated
|
|
||
| const startHeight = isOpen ? openHeight.current : closedHeight.current; | ||
| const startHeight = useMemo(() => isOpen ? openHeight.current : closedHeight, [closedHeight]); |
There was a problem hiding this comment.
Actually I'm not sure memoizing this is a good idea as it affects other non-memoized consts
There was a problem hiding this comment.
it affects memoized consts or is used as an initializer for useRef references, also added the missing dep
src/timeline-list/index.tsx
Outdated
| } | ||
| } | ||
| prevDate.current = date; | ||
| }, [pagesRef.current, updateSource]); |
There was a problem hiding this comment.
pagesRef.current is not dep...
There was a problem hiding this comment.
why not? it changes when the number of days changes (since when numOfDay = undefined we have a page for each day and when the number of days = 3 we have a page for every 3 days), when the scroll doesn't adjust to it we get an issue with the scrolling (it thinks that it scrolled 3 pages instead of 1 because of the pagesRef old value being used
There was a problem hiding this comment.
Mutable values like 'something.current' aren't valid dependencies because mutating them doesn't re-render the component.
There was a problem hiding this comment.
removed unnecessary dependency
|
@yuvalsho I don't understand how the changes in Expandable/index regarding the height and closed mechanized relate to this PR? Are there other issues you're fixing here? |
src/dateutils.ts
Outdated
| } | ||
| } | ||
|
|
||
| export function sameDayRange({ |
There was a problem hiding this comment.
'onSameDateRange', no?
| useDidUpdate(() => { | ||
| if (updateSource !== UpdateSources.WEEK_SCROLL) { | ||
| const pageIndex = items.current.findIndex(item => sameWeek(item, date, firstDay)); | ||
| const pageIndex = items.current.findIndex( | ||
| item => (numberOfDays && numberOfDays > 1) ? |
There was a problem hiding this comment.
Better move this condition (numberOfDays && numberOfDays > 1) to a const since you're using the same one later in line 250
There was a problem hiding this comment.
extracted to a function (it's used outside of the component so it cant be a const/memoized)
src/day-state-manager.ts
Outdated
| @@ -3,10 +3,10 @@ const {toMarkingFormat} = require('./interface'); | |||
|
|
|||
|
|
|||
| export function getState(day: XDate, current: XDate, props: any) { | |||
| const {minDate, maxDate, disabledByDefault, context} = props; | |||
| const {minDate, maxDate, disabledByDefault, context, disableDaySelection} = props; | |||
There was a problem hiding this comment.
Why is disableDaySelection part of the 'props' if it's not a prop? It's confusing, why not add an option object or another pharam?
There was a problem hiding this comment.
i agree, moved to be a different parameter
src/dateutils.ts
Outdated
| } | ||
| } | ||
|
|
||
| export function onSameDAteRange({ |
There was a problem hiding this comment.
The "A" on Date is capitalized... typo I guess
fixed issues: