Add ability to upper case week days text by setting upperCaseWeekDays#392
Add ability to upper case week days text by setting upperCaseWeekDays#392vanyasem wants to merge 1 commit intohyochan:mainfrom
upperCaseWeekDays#392Conversation
hyochan
left a comment
There was a problem hiding this comment.
I agree that while providing a CustomWeekdayBuilder to build Weekdays in a different form is useful, adding upperCaseWeekDays for simpler modifications is a good idea. However, it might be clearer if an enum type that supports this kind of formatting were provided as well. Additionally, I think it would be helpful to explicitly state that the priority of this approach is lower than that of CustomWeekdayBuilder. What are your thoughts on this?
Are you talking about including it in the
Absolutely agree on that one |
In this case |
|
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 |
|
The PR is not stale! |
|
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 |
|
Still planning on continuing this some time later |
|
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 |
|
Still planning on continuing this some time later |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an upperCaseWeekDays property to the CalendarCarousel and WeekdayRow components to allow for uppercase weekday labels. Feedback suggests applying this transformation to the weekday names before they are passed to custom builders for consistency and renaming the property to upperCaseWeekdays within the WeekdayRow class to match existing naming conventions.
| style: defaultWeekdayTextStyle, | ||
| child: Text( | ||
| weekDayName, | ||
| upperCaseWeekDays ? weekDayName.toUpperCase() : weekDayName, |
There was a problem hiding this comment.
The upperCaseWeekDays transformation is currently only applied to the default Text widget. If a customWeekdayBuilder is provided, it receives the original weekDayName string without the uppercase transformation. For a more consistent user experience, the transformation should ideally be applied to the string before it is passed to the custom builder as well. Consider refactoring the logic to apply the casing transformation in _renderWeekDays before calling _weekdayContainer.
| final TextStyle? weekdayTextStyle; | ||
| final DateFormat localeDate; | ||
| final int firstDayOfWeek; | ||
| final bool upperCaseWeekDays; |
There was a problem hiding this comment.
In WeekdayRow, existing properties such as showWeekdays, weekdayFormat, and weekdayTextStyle use a lowercase 'd' for 'weekday'. To maintain internal consistency within this file, consider naming this property upperCaseWeekdays (lowercase 'd'), even though CalendarCarousel uses an uppercase 'D' for showWeekDays.
Bot review pass 1
No blockers. Merge is still gated by:
Classification: |
hyochan
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Walked through the diff carefully. Two blockers and a small polish item before merge.
Blocker — customWeekdayBuilder path doesn't get uppercased
In lib/src/weekday_row.dart the .toUpperCase() only runs inside the non-custom-builder branch of _weekdayContainer (line 46 post-patch). Users who pass customWeekdayBuilder will still receive the original-case weekDayName even when upperCaseWeekDays: true. This is the same issue Gemini flagged.
Suggested fix: transform once in _renderWeekDays() before calling _weekdayContainer, e.g.:
weekDay = upperCaseWeekDays ? weekDay.toUpperCase() : weekDay;
list.add(_weekdayContainer(count, weekDay));That way both the default path and the custom-builder path see the uppercased string, consistently.
Blocker — naming convention mismatch on the internal WeekdayRow field
The public field on CalendarCarousel (upperCaseWeekDays, capital D) matches the existing showWeekDays/weekDayFormat/weekDayMargin convention on that widget — that's fine, keep it.
But inside WeekdayRow everything else is lowercase d (showWeekdays, weekdayFormat, weekdayMargin, weekdayPadding, weekdayTextStyle). Please rename the new internal field to upperCaseWeekdays to match. Public API stays upperCaseWeekDays; only the WeekdayRow constructor parameter / field changes.
Polish — dartdoc
Per our contribution rules, every new public parameter on CalendarCarousel needs a /// dartdoc comment. One line is enough, e.g.:
/// Whether to render weekday labels in uppercase.
final bool upperCaseWeekDays;Nice-to-have (not blocking)
A widget test confirming uppercase rendering with both the default path and a custom WeekdayBuilder would nail down the behavior.
Push the fixes and I'll re-review. 👍
Non-breaking change
Adds support for an optional parameter
upperCaseWeekDays, which makes weekDays text uppercase