Feature/weekend days#576
Conversation
- add holiday Days feature - update gradle
-disabled weekend days -update grade
-disabled weekend days - update gradle
|
This looks like a good feature to have. |
|
Thanks , I wait for review my commit
…On Tue, Apr 9, 2019, 6:33 PM wdullaer ***@***.***> wrote:
This looks like a good feature to have.
I will try to do a detailed review later this week.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#576 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/Au4hey1_PLtdYMjDkIt9Ud8q-wKetiBYks5vfMDXgaJpZM4cWEWn>
.
|
wdullaer
left a comment
wdullaer
left a comment
There was a problem hiding this comment.
You did not update the DefaultDateRangeLimiter test suite. You should provide some unit and property tests that prove that the interactions of the disabled weekdays with the existing functionality work as expected.
| } | ||
| dependencies { | ||
| classpath 'com.android.tools.build:gradle:3.2.1' | ||
| classpath 'com.android.tools.build:gradle:3.3.2' |
There was a problem hiding this comment.
Please don't update gradle in a feature PR, unless it's required for the functionality you are building.
This avoids merge conflicts if gradle gets upgraded in master before your PR lands
| * | ||
| * @param weekendDays an Array of Calendar Objects containing the disabled dates | ||
| */ | ||
| public void setWeekendDays(@NonNull List<Integer> weekendDays) { |
There was a problem hiding this comment.
I don't like the naming here. There's nothing about a weekend day which implies that it's disabled. Some people might have the exact opposite requirement.
Maybe setDisabledWeekDays?
| * | ||
| * @param weekendDays an Array of Calendar Objects containing the disabled dates | ||
| */ | ||
| public void setWeekendDays(@NonNull List<Integer> weekendDays) { |
There was a problem hiding this comment.
Maybe we can use this in the signature? https://docs.oracle.com/javase/8/docs/api/java/time/DayOfWeek.html (it has utility methods to convert to a Calendar int later on).
That might be a more intuitive signature than int.
| @Override | ||
| public @NonNull Calendar getStartDate() { | ||
| public @NonNull | ||
| Calendar getStartDate() { |
There was a problem hiding this comment.
You need to make sure the start date is not on a disabled weekday
| @Override | ||
| public @NonNull Calendar getEndDate() { | ||
| public @NonNull | ||
| Calendar getEndDate() { |
There was a problem hiding this comment.
You need to make sure the end date is not on a disabled weekday
| @Override | ||
| public @NonNull Calendar setToNearestDate(@NonNull Calendar calendar) { | ||
| public @NonNull | ||
| Calendar setToNearestDate(@NonNull Calendar calendar) { |
There was a problem hiding this comment.
setToNearest will need to take the disabled weekdays into account (you don't want to return a date that is disabled)
| android:layout_height="wrap_content"> | ||
|
|
||
| <LinearLayout | ||
| android:orientation="vertical" android:layout_width="match_parent" |
| private Calendar mMaxDate; | ||
| private TreeSet<Calendar> selectableDays = new TreeSet<>(); | ||
| private HashSet<Calendar> disabledDays = new HashSet<>(); | ||
| private List<Integer> weekendDays = new ArrayList<>(); |
There was a problem hiding this comment.
Maybe a set makes more sense than a list?
3 participants
-disabled weekend days