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

Apply timetable item(session) detail design roughly #85

Merged
merged 22 commits into from
Jul 3, 2024

Conversation

Yamada-Ika
Copy link
Contributor

@Yamada-Ika Yamada-Ika commented Jul 1, 2024

Issue

Overview (Required)

  • I've implemented the timetable detailed screen according to the design in Figma, through some details remain unfinished.

Links

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

Before After

Movie (Optional)

Before After
2024-07-02.23.41.01.mov
2024-07-02.23.42.14.mov

@Yamada-Ika Yamada-Ika self-assigned this Jul 1, 2024
@takahirom
Copy link
Member

takahirom commented Jul 2, 2024

Thanks. I'll take a look shortly!

Copy link
Contributor Author

@Yamada-Ika Yamada-Ika left a comment

Choose a reason for hiding this comment

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

Additional comment🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed directory to enable the use of vector images

Copy link
Member

Choose a reason for hiding this comment

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

Nice!

modifier: Modifier = Modifier,
) {
// TODO: switch color according to room type
TopAppBar(
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'm not using LargeTopAppBar. Implementing this on the title would disrupt the layout. So, I implemented to TimeTableItemDetailHeadline 🙇

Copy link
Member

Choose a reason for hiding this comment

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

👍

TopAppBar(
modifier = modifier,
colors = TopAppBarDefaults.topAppBarColors().copy(
containerColor = Color(0xFF132417),
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 haven't implemented the logic to switch colors based on room type 🙇
hardcoded the color for Arctic Fox...

@Composable
@MultiThemePreviews
@MultiLanguagePreviews
fun TimetableItemDetailTopAppBarUnSelectablePreview() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are others also experiencing the issue where the Compose Preview cannnot be displayed in Android Studio (with the icon not appearing as shown in picture)?

スクリーンショット 2024-07-03 0 02 30

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I know this problem. You can refer to https://speakerdeck.com/ogi2ogi/get-started-with-compose-multiplatform?slide=24 for more information.
There are some solutions, but generally, we can't see the preview using Android Studio with commonMain.

I recommend running ./gradlew recordRoborazziDebug and using the Roborazzi Idea Plugin.
You can find it here: https://plugins.jetbrains.com/plugin/24561-roborazzi

@Yamada-Ika Yamada-Ika marked this pull request as ready for review July 2, 2024 15:06
Modifier.padding(
top = innerPadding.calculateTopPadding(),
),
LazyColumn(
Copy link
Member

Choose a reason for hiding this comment

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

👍

@takahirom takahirom requested a review from a team July 3, 2024 02:07
Text(
text = "続きを読む",
style = MaterialTheme.typography.labelLarge,
color = Color(0xFF45E761),
Copy link
Member

Choose a reason for hiding this comment

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

It's OK to merge this but could you add comments for each Colors that we could fix this after?

// FIXME: Implement and use a theme color instead of fixed colors like RoomColors.primary and RoomColors.primaryDim

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. I handled it with this PR 🙇
#90

Copy link
Member

@takahirom takahirom left a comment

Choose a reason for hiding this comment

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

Looks great! I believe this is definitely better than what we had before!

@takahirom takahirom merged commit 7b31ab8 into main Jul 3, 2024
2 checks passed
@takahirom takahirom deleted the yamada-ika/brush-up-timetable-item-detail-screen branch July 3, 2024 02:25
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.

Apply timetable item(session) detail design roughly
2 participants