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

Week mode #89

Closed
wants to merge 13 commits into from
Closed

Week mode #89

wants to merge 13 commits into from

Conversation

andrey-ananiev
Copy link

@andrey-ananiev andrey-ananiev commented Feb 14, 2023

I added a weekly mode to the calendar.
Closes #39

@boguszpawlowski
Copy link
Owner

Hi @andrey-ananiev. Thank you for your contribution! I'm a little short on time right now, but I should be able to review this on the weekend. Until then, please make sure that Detekt is passing and run ./gradlew apiDump to generate new API scheme.

@andrey-ananiev
Copy link
Author

Hi. I don`t know how I can retate the Detect Check. I am new to github.

Copy link
Owner

@boguszpawlowski boguszpawlowski left a comment

Choose a reason for hiding this comment

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

Overall, I think it would be beneficial to change the approach a bit, and create a separate WeekCalendar composable. This would require a bit more of duplication but would allow us to simplify the logic which is pretty complicated right now. I think you could reuse WeekPager and WeekListState and after that it shouldn't be too much work.

val daysOfWeek = remember(firstDayOfWeek) {
DayOfWeek.values().rotateRight(DaysOfWeek - firstDayOfWeek.ordinal)
}

Column(
modifier = modifier,
modifier = modifier
.animateContentSize(),
Copy link
Owner

Choose a reason for hiding this comment

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

I wouldn't add additional parameters to the modifier here. If the use-case requires such behavior, you can easily pass your own modifier from outside this function. The library should be as unopinionated as possible to fit the largest amount of possible use-cases.

Copy link
Author

Choose a reason for hiding this comment

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

Fine.

@@ -13,4 +14,5 @@ public typealias NonSelectableDayState = DayState<EmptySelectionState>
public data class DayState<T : SelectionState>(
private val day: Day,
val selectionState: T,
val eventState: EventState
Copy link
Owner

Choose a reason for hiding this comment

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

I believe adding such functionality shouldn't be in scope of this PR.

Copy link
Author

@andrey-ananiev andrey-ananiev Feb 25, 2023

Choose a reason for hiding this comment

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

In my project, I display the events of the day as dots in the calendar. Why not add this functionality to this PR?

Copy link
Owner

Choose a reason for hiding this comment

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

Because this is easily achievable via the existing library API, and adding additional parameters to the API increases its complexity and reduces readability. Also, not every project will want to represent events as dots.

weekContainer: @Composable (content: @Composable (PaddingValues) -> Unit) -> Unit = { content ->
Box { content(PaddingValues()) }
},
onSwipe: (LocalDate) -> Unit = { },
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Author

Choose a reason for hiding this comment

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

When the month changes, I load data to display dots representing events.

Comment on lines 54 to +55
.replaceFirstChar { it.titlecase() },
style = MaterialTheme.typography.h4,
style = MaterialTheme.typography.h5,
Copy link
Owner

Choose a reason for hiding this comment

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

What is the reason for this change?

Copy link
Author

Choose a reason for hiding this comment

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

I think H4 is too big.

package io.github.boguszpawlowski.composecalendar.week
package io.github.boguszpawlowski.composecalendar.header
Copy link
Owner

Choose a reason for hiding this comment

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

Why changing the package?

Copy link
Author

Choose a reason for hiding this comment

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

I thought it made more sense.




//@Suppress("FunctionName") // Factory function
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove commented code.

Copy link
Author

Choose a reason for hiding this comment

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

I'm sorry, I'll delete it.

Comment on lines +16 to +17
@Stable
internal class WeekListState(
Copy link
Owner

Choose a reason for hiding this comment

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

This and WeekPager could be easily used in order to create separate WeekCalendar composable.

Copy link
Author

Choose a reason for hiding this comment

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

Why do it separately when you can combine it?

internal fun LocalDate.getWeek(
today: LocalDate = LocalDate.now(),
): Week {
val firstDayOfWeek = minusDays(this.dayOfWeek.value.toLong() - 1)
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this take into account the firstDayOfTheWeek parameter?

Copy link
Author

Choose a reason for hiding this comment

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

I'm sorry... I'll fix it. I will definitely add this option.

Comment on lines +41 to +60
val scrollState = rememberScrollState()
Column(modifier = Modifier
.fillMaxWidth()
.verticalScroll(scrollState)
) {
val calendarState = rememberCalendarState(
eventState = EventState( dayEventList )
)
ModeControls(modeState = calendarState.modeState)
StaticCalendar(
modifier = Modifier.animateContentSize(),
showAdjacentMonths = false,
firstDayOfWeek = SUNDAY,
monthContainer = { MonthContainer(it) },
dayContent = { DayContent(dayState = it) },
weekDaysNames = { DefaultWeekDaysNames(listDays = it) },
monthHeader = { MonthHeader(currentState = it) },
calendarState = calendarState
)
}
Copy link
Owner

Choose a reason for hiding this comment

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

This cuould be in a separate WeekCalendar sample

Copy link
Author

Choose a reason for hiding this comment

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

I see that you are doing it in the next fork as you see fit. Tomorrow or the day after tomorrow I will have time to look at your varant in detail.

val calendarState = rememberMonthSelectionState(
eventState = EventState( dayEventList )
)
ModeControls(modeState = calendarState.modeState)
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think adding mode controls to every sample is necessary

Copy link
Author

Choose a reason for hiding this comment

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

Fine.

@boguszpawlowski
Copy link
Owner

boguszpawlowski commented Feb 19, 2023

Hi. I don`t know how I can retate the Detect Check. I am new to github

@andrey-ananiev If you have a mac, you can run ./gradlew detekt (or gradlew detekt on Windows) locally in a terminal in project directory. This should show you all the errors from detekt.

@boguszpawlowski
Copy link
Owner

@andrey-ananiev I have implemented something similar, using 2 separate composables, would be grateful if you would take a look and tell me what do you think about it: #91
Personally, I think this will be much easier to maintain in the future and the resulting code is much more readable.

@andrey-ananiev
Copy link
Author

@andrey-ananiev I have implemented something similar, using 2 separate composables, would be grateful if you would take a look and tell me what do you think about it: #91 Personally, I think this will be much easier to maintain in the future and the resulting code is much more readable.

Thank you for supporting the weekly calendar idea. I will definitely look at your implementation and give feedback.

@boguszpawlowski
Copy link
Owner

@andrey-ananiev Thank you for your contribution, but I will be going with the second pr.

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.

Week view
2 participants