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

Add disableMonthScroll #2524

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

LeXuanKhanh
Copy link

The current expandable calendar is missing properties to disable scroll when it is in the opened state (like CalendarList). This implementation will resolve that issue and also provide some workaround for some bugs

@@ -580,7 +583,7 @@ const ExpandableCalendar = (props: ExpandableCalendarProps) => {
onDayPress={_onDayPress}
onVisibleMonthsChange={onVisibleMonthsChange}
pagingEnabled
scrollEnabled={isOpen}
scrollEnabled={isOpen && !disableMonthScroll}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since CalendarList props allow passing ScrollView's scrollEnabled I prefer to use this one instead of adding another prop to the API.
Let's try moving line 586 before the {...others} (line 579) to allow overriding the 'scrollEnabled' prop by passing it instead of 'disableMonthScroll' to the ExpandableCalendar component.

Copy link
Author

Choose a reason for hiding this comment

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

Do you think it will confuse the user because we already had the disableWeekScroll prop ? They will think that using the prop scrollEnabled will disable scrolling behavior for both states opened and closed, not only opened. For that, I think we should use another prop for clarity.
If you insist on making the change in your opinion please let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand your point, but in the case of 'disableWeekScroll' we change the component from WeekCalendar to a simple Week component. In your implementation you're not replacing the CalendarList with a simple Calendar component (which is an option), but only passing it to the CalendarList's FlatList implementation.

Copy link
Author

Choose a reason for hiding this comment

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

I tried to do the same logic as the WeekCalendar but seems using the normal Calendar component it will break. So in this case must we use the scrollEnabled ?

Copy link
Author

Choose a reason for hiding this comment

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

any update @Inbal-Tish ?

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.

2 participants