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.

Sign up for GitHub

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

Jump to bottom

Expanded Lunch Field (Grid) #712

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Expanded Lunch Field (Grid) #712

wants to merge 8 commits into from
+66 −13

Conversation

charles-b-stb
Copy link
Contributor

Issue

Overview (Required)

  • Added in special lunch implementation for grid view (stretched, special color, special icon). This now should match the iOS implementation.

Links

Screenshot (Optional if screenshot test is present or unrelated to UI)

Before After

Movie (Optional)

Before After

@github-actions GitHub Actions
Copy link

Detekt check failed. Please run ./gradlew detekt --auto-correct to fix the issues.

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 21, 2024 13:10 Inactive
@charles-b-stb
Copy link
Contributor Author

Fixed formatting. Ready for review.

@github-actions GitHub Actions
Copy link

Detekt check failed. Please run ./gradlew detekt --auto-correct to fix the issues.

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 24, 2024 04:59 Inactive
takahirom
takahirom reviewed Aug 24, 2024
View reviewed changes
...src/commonMain/kotlin/io/github/droidkaigi/confsched/sessions/component/TimetableGridItem.kt
@@ -106,7 +106,7 @@ fun TimetableGridItem(
derivedStateOf { (height - TimetableGridItemSizes.padding * 2) / 4 > titleMinHeightDp * 3 / 2 }
}

ProvideRoomTheme(timetableItem.room.getThemeKey()) {
Copy link
Member

Choose a reason for hiding this comment

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

Our formatter says this. Could you check it out?

/home/runner/work/conference-app-2024/conference-app-2024/feature/sessions/src/commonMain/kotlin/io/github/droidkaigi/confsched/sessions/component/TimetableGridItem.kt:109:22: Extra braces exist on this branch, remove them. [BracesOnIfStatements]
/home/runner/work/conference-app-2024/conference-app-2024/feature/sessions/src/commonMain/kotlin/io/github/droidkaigi/confsched/sessions/component/TimetableGridItem.kt:109:61: Extra braces exist on this branch, remove them. [BracesOnIfStatements]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird. I ran the format fixer....

I will try to fix this.

@charles-b-stb
Copy link
Contributor Author

@takahirom fixed (again)

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 24, 2024 15:03 Inactive
takahirom
takahirom reviewed Aug 25, 2024
View reviewed changes
...src/commonMain/kotlin/io/github/droidkaigi/confsched/sessions/component/TimetableGridItem.kt Outdated
@@ -106,7 +106,7 @@ fun TimetableGridItem(
derivedStateOf { (height - TimetableGridItemSizes.padding * 2) / 4 > titleMinHeightDp * 3 / 2 }
}

ProvideRoomTheme(timetableItem.room.getThemeKey()) {
ProvideRoomTheme(if (timetableItem.isLunch) "lunch" else timetableItem.room.getThemeKey()) {
Copy link
Member

Choose a reason for hiding this comment

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

How about passing isLunch to getThemeKey()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@takahirom
Copy link
Member

I would like to have a fake session for lunch which will be used for screenshot tests. It could break unexpectedly otherwise.

@github-actions GitHub Actions
Copy link

Detekt check failed. Please run ./gradlew detekt --auto-correct to fix the issues.

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 25, 2024 16:23 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution August 25, 2024 16:33 Inactive
@github-actions GitHub Actions
Copy link

Detekt check failed. Please run ./gradlew detekt --auto-correct to fix the issues.

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 25, 2024 16:47 Inactive
takahirom
takahirom reviewed Aug 26, 2024
View reviewed changes
core/model/src/commonMain/kotlin/io/github/droidkaigi/confsched/model/Timetable.kt Outdated
jaTitle = "${fake.title.jaTitle} $day $index",
enTitle = "${fake.title.enTitle} $day $index",

if(start == DroidKaigi2024Day.Workday.start + 13.hours) {
Copy link
Member

Choose a reason for hiding this comment

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

There may not be a DroidKaigi 2024 Day 1 session starting at 1 PM.
I think we can place this above the for-loop.
Could you please check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I will look now. Sorry about yesterday (I was unwell yesterday and needed to rest)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@takahirom if you mean day zero... do we need test data for that?

We cut day zero from the timetable.

Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need this if statement, and we can move the add statement outside of the for-loop.
Additionally, I believe we should use DroidKaigi2024Day.ConferenceDay1.start instead of DroidKaigi2024Day.Workday.start for startsAt and endsAt.

@charles-b-stb
Copy link
Contributor Author

Adjusted as requested. 🙇

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 27, 2024 12:43 Inactive
@github-actions GitHub Actions
Copy link

Snapshot diff report

File name Image
TimetablePreview_com
pare.png
TimetableVerticalSca
le60PercentPreview_c
ompare.png
TimetableVerticalSca
le20PercentPreview_c
ompare.png
TimetableVerticalSca
le40PercentPreview_c
ompare.png

takahirom
takahirom reviewed Aug 27, 2024
View reviewed changes
core/model/src/commonMain/kotlin/io/github/droidkaigi/confsched/model/Timetable.kt Outdated
TimetableItem.Special(
id = TimetableItemId("7"),
title = MultiLangText("Lunch break", "Lunch break"),
startsAt = DroidKaigi2024Day.Workday.start + 13.hours,
Copy link
Member

Choose a reason for hiding this comment

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

I think this is still DroidKaigi2024Day.Workday 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, sorry I forgot to fix that part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@takahirom sorry about that. Fixed as requested.

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 27, 2024 13:42 Inactive
@charles-b-stb
Copy link
Contributor Author

Additional requested actions:

  • Add an @Preview test for lunch display
  • Add additional tests (unit tests?) to test the code for verifying the special lunch display.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@takahirom takahirom Awaiting requested review from takahirom

Assignees
No one assigned
Labels
Waiting for response or commit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stretch "Lunch" Graph item like iOS
2 participants
@charles-b-stb @takahirom