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

Extract the timetable item UI component as TimeTableItemCard and move it to the core:ui module #234

Merged
merged 16 commits into from
Aug 7, 2024
Merged

Extract the timetable item UI component as TimeTableItemCard and move it to the core:ui module #234

merged 16 commits into from
Aug 7, 2024

Conversation

Yamada-Ika
Copy link
Contributor

@Yamada-Ika Yamada-Ika commented Aug 5, 2024 •
edited by takahirom
Loading

close #215

Overview (Required)

  • I extracted the UI component that is commonly used in the feature:favorite module (to be added in a separate PR) and the feature:sessions module into the core:ui module.
  • I roughly implemented the timetable item card UI based on Figma design.

Links

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

Before After

Movie (Optional)

Before After

@Yamada-Ika Yamada-Ika self-assigned this Aug 5, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 5, 2024

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

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 5, 2024 16:30 Inactive
@Yamada-Ika Yamada-Ika changed the title Extract the timetable item UI component as TimeTableItemCard and move it to the core ui module Extract the timetable item ui component as TimeTableItemCard and move it to the core ui module Aug 5, 2024
@github-actions github-actions bot temporarily deployed to deploygate-distribution August 5, 2024 16:35 Inactive
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 5, 2024

Snapshot diff report

File name Image
TimetableScreenTest[
TimetableScreen - wh
en server is operati
onal - click first s
ession bookmark - it
should show bookmar
ked session]_compare
.png
TimetableScreenTest[
TimetableScreen - wh
en server is operati
onal - it should sho
w timetable items]_c
ompare.png
KaigiAppTest.checkSt
artupShot_compare.pn
g
KaigiAppTest.checkMe
diumTabletLaunchShot
_compare.png

@Yamada-Ika Yamada-Ika changed the title Extract the timetable item ui component as TimeTableItemCard and move it to the core ui module Extract the timetable item UI component as TimeTableItemCard and move it to the core:ui module Aug 5, 2024
Yamada-Ika
Yamada-Ika commented Aug 5, 2024
View reviewed changes
core/ui/src/commonMain/kotlin/io/github/droidkaigi/confsched/ui/component/TimeTableItemCard.kt Outdated
fun TimeTableItemCard(
isBookmarked: Boolean,
timetableItem: TimetableItem,
tags: @Composable RowScope.() -> Unit,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The timetable item card on the sessions screen and the favorites screen have slightly different tags.
To allow customization for each screen, slots have been provided.

sessions screen favorites screen

Copy link
Member

Choose a reason for hiding this comment

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

Looks great.

@Yamada-Ika Yamada-Ika marked this pull request as ready for review August 5, 2024 18:51
takahirom
takahirom reviewed Aug 6, 2024
View reviewed changes
core/ui/src/commonMain/kotlin/io/github/droidkaigi/confsched/ui/component/TimeTableItemTag.kt Outdated
import androidx.compose.ui.unit.dp

@Composable
fun TimeTableItemTag(
Copy link
Member

Choose a reason for hiding this comment

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

Thanks. It's a very minor point, but 'TimeTable' should be 'Timetable' for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had mistakenly named files and functions related to the timetable detail screen as TimeTable, so I have corrected them to Timetable 🙇
39cfb84

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 6, 2024 17:43 Inactive
@takahirom
Copy link
Member

Looks great.
Just one thing: Is there any reason you used ui_strings.xml instead of strings.xml? I've renamed it. If you have any thoughts on this, please let me know.
I don't have any use case for strings.xml other than UI, so I don't think we need to use the ui_ prefix.

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 7, 2024 02:22 Inactive
@takahirom takahirom merged commit 294c98e into main Aug 7, 2024
5 checks passed
@takahirom takahirom deleted the yamada-ika/brush-up-timetable-item-card branch August 7, 2024 02:42
@Yamada-Ika
Copy link
Contributor Author

I used other modules as a reference and added the module name as a prefix.
I agree with your points, so I think using strings.xml is a good idea. 🙇

@takahirom
Copy link
Member

I might make mistake. I'll take a look at it. Thanks!

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

@Yamada-Ika Yamada-Ika

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Timetable item border color is white
2 participants
@Yamada-Ika @takahirom