Support specifying inactive dates#395
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for specifying dates that should be rendered and treated as inactive (unselectable) in the calendar widget.
- Introduces a new
inactiveDatesproperty with a default empty list - Updates day rendering and tap handling to respect
inactiveDates - Adds a
DateTime.isSameDayextension to compare dates without time components
Comments suppressed due to low confidence (2)
lib/flutter_calendar_carousel.dart:128
- Public API fields like
inactiveDatesshould have doc comments explaining their purpose and behavior (e.g., how dates are matched and what rendering changes occur).
final List<DateTime> inactiveDates;
lib/flutter_calendar_carousel.dart:871
- The new inactive-date blocking logic in
_onDayPressedshould have corresponding unit or widget tests to verify that taps on inactive dates do not select those days.
if (widget.inactiveDates.any(
| } else if (widget.inactiveDates.any( | ||
| (final DateTime inactiveDate) => | ||
| inactiveDate.isSameDay(now), | ||
| )) { | ||
| isSelectable = false; | ||
| } | ||
|
|
There was a problem hiding this comment.
The inactiveDates.any(...) check is duplicated in multiple rendering branches. Consider extracting this into a helper method (e.g. bool _isInactive(DateTime day)) to reduce duplication and improve readability.
| } else if (widget.inactiveDates.any( | |
| (final DateTime inactiveDate) => | |
| inactiveDate.isSameDay(now), | |
| )) { | |
| isSelectable = false; | |
| } | |
| } else if (_isInactive(now)) { |
| } else if (now.millisecondsSinceEpoch > | ||
| maxDate.millisecondsSinceEpoch) { | ||
| isSelectable = false; | ||
| } else if (widget.inactiveDates.any( |
There was a problem hiding this comment.
Using List.any for each day render can be O(n) per cell; if inactiveDates grows large, consider storing dates in a Set or hashing normalized date keys for constant-time lookups.
There was a problem hiding this comment.
not going to address this
|
This PR is stale because it has been open 90 days with no activity. Leave a comment or this will be closed in 7 days |
|
@hyochan sorry for the lengthly delay. I've replied to the Copilot's review. Would you mind taking an another look at the PR? |
|
This PR is stale because it has been open 90 days with no activity. Leave a comment or this will be closed in 7 days |
|
not stale |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an inactiveDates property to the CalendarCarousel widget, allowing specific dates to be disabled. The logic for determining date selectability has been refactored into a centralized _determineIsSelectable method, supported by a new DateTime extension. Review feedback suggests simplifying the _determineIsSelectable method with early returns to improve readability and potentially optimizing lookup performance with a Set. Additionally, a redundant check in the _onDayPressed handler was noted, with a suggestion to use the helper method for consistency.
| bool _determineIsSelectable(DateTime now) { | ||
| bool isSelectable = true; | ||
| if (now.millisecondsSinceEpoch < minDate.millisecondsSinceEpoch) { | ||
| isSelectable = false; | ||
| } else if (now.millisecondsSinceEpoch > maxDate.millisecondsSinceEpoch) { | ||
| isSelectable = false; | ||
| } else if (widget.inactiveDates.any( | ||
| (final DateTime inactiveDate) => inactiveDate.isSameDay(now), | ||
| )) { | ||
| isSelectable = false; | ||
| } | ||
| return isSelectable; | ||
| } |
There was a problem hiding this comment.
The _determineIsSelectable method can be simplified by using early returns and returning the result of the any check directly. This improves readability and reduces the need for a mutable variable.
Additionally, note that performing an any check on a List for every day rendered (35-42 times per month) results in O(N) complexity per day. If inactiveDates is expected to be large, consider pre-processing it into a Set of normalized values (e.g., in initState) to achieve O(1) lookups.
bool _determineIsSelectable(DateTime now) {
if (now.millisecondsSinceEpoch < minDate.millisecondsSinceEpoch) return false;
if (now.millisecondsSinceEpoch > maxDate.millisecondsSinceEpoch) return false;
return !widget.inactiveDates.any(
(final DateTime inactiveDate) => inactiveDate.isSameDay(now),
);
}| if (widget.inactiveDates.any( | ||
| (final DateTime inactiveDate) => inactiveDate.isSameDay(picked), | ||
| )) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This check is redundant because renderDay already uses isSelectable (which calls _determineIsSelectable) to set the onPressed callback to null for inactive dates. If you prefer to keep this as a defensive check, it should ideally call _determineIsSelectable(picked) to ensure consistency and avoid duplicating the logic.
Bot review pass 1
No blockers. Merge is still gated by:
Classification: |
hyochan
left a comment
hyochan
left a comment
There was a problem hiding this comment.
Thanks — nice refactor to centralize the selectability check. A couple of blockers and polish items before merge.
Blocker — _onDayPressed duplicates the inactive check
After this diff, lib/flutter_calendar_carousel.dart has two copies of the "is this day blocked?" logic: _determineIsSelectable(...) used by the render paths, and an open-coded min/max + inactiveDates.any(...) inline in _onDayPressed (lines 857–864). Please consolidate — this is exactly what Gemini's medium-priority #2 was pointing at, and leaving both in place guarantees drift when the rules change.
Suggested fix:
void _onDayPressed(DateTime picked) {
if (!_determineIsSelectable(picked)) return;
setState(() {
_selectedDate = picked;
});
widget.onDayPressed
?.call(picked, widget.markedDatesMap?.getEvents(picked) ?? const []);
}Blocker — dartdoc on the new public parameter
Per our contribution rules, every new public parameter on CalendarCarousel needs a /// dartdoc comment. For inactiveDates please document both the intent and the timezone caveat, since isSameDay compares year/month/day on whatever calendar the DateTime is in:
/// Dates that should be rendered as non-selectable (pressing them is ignored).
///
/// Comparison is done by `year`, `month`, and `day`, so pass `DateTime` values
/// in the same timezone as the rest of your calendar data to avoid off-by-one
/// day mismatches across DST / UTC boundaries.
final List<DateTime> inactiveDates;Polish
- Name the extension. The anonymous
extension on DateTimeat the bottom of the file is file-scoped and works, but a name (e.g._DateTimeSameDay) makes intent clearer and avoids any future surprise if the file grows more extensions. - Early returns in
_determineIsSelectable. Minor; per Gemini's suggestion, you can turn the bool-accumulator pattern into straightreturn false;branches — purely readability.
Acknowledging the perf thread
Copilot's earlier List.any → Set suggestion: I'm OK leaving this as-is for the common case (small inactiveDates, ~35–42 cells). If downstream users hit scale problems we can revisit in a follow-up.
Nice-to-have (not blocking)
A widget test that (a) renders a day listed in inactiveDates and confirms it's styled as non-selectable, and (b) taps it and confirms onDayPressed is not fired.
Push the fixes and I'll re-review. 🙏
Non-breaking change
Closes #381
Closes #286