-
Notifications
You must be signed in to change notification settings - Fork 201
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
Grid-like session list (timetable / favorites) #693
Grid-like session list (timetable / favorites) #693
Conversation
I think we should run a test for tablet devices. Could you try implementing this? |
@takahirom |
Detekt check failed. Please run |
override fun setQualifier(qualifier: String) { | ||
RuntimeEnvironment.setQualifiers(qualifier) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tried to add function for RobolectricDeviceQualifier programmatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing Robot separates a test's 'what' and 'how'. The 'qualifier' is how we set up the device or configure settings. So I think it should not be in the test. How about naming it DeviceSetupRobot? And having a method called setupTabletDevice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for my lack of understanding 🙇
I renamed class name and fun name as above 🙏
commit: 7ddc3bd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, but we don't want to use "qualifier" as a parameter. For example, we could use an emulator instead. In that pattern, we need to use other APIs for the emulator. In such cases, we don't have a qualifier. Also, testers might find it difficult to understand what a qualifier is. What I want to have is just "setupTabletDevice()". 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So sorry again 🙇 🙇
I DID misread setupTablet
Device as setupTest
Device 💀
I modified this function name and params 🙏
commit: cc228b4
Detekt check failed. Please run |
@@ -119,7 +120,7 @@ fun TimetableItemCard( | |||
|
|||
timetableItem.speakers.forEach { speaker -> | |||
Row( | |||
modifier = Modifier.padding(top = 4.dp), | |||
modifier = Modifier.height(36.dp).padding(top = 4.dp), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to enable measure component height correctly even if we use IntrinsicSize.Max
,
add height(...)
explicitly.
modifier = Modifier.padding(top = 8.dp), | ||
modifier = Modifier | ||
.padding(top = 8.dp) | ||
.height(IntrinsicSize.Min), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ the almost all screenshot differences of TimetableScreen are because of the code when I was debugging 🙏 |
@@ -30,39 +39,54 @@ fun FavoriteList( | |||
modifier: Modifier = Modifier, | |||
contentPadding: PaddingValues = PaddingValues(), | |||
) { | |||
val isWideWidthScreen = LocalIsWideWidthScreen.current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! How about using WindowWidthSizeClass directly here instead of providing a CompositionLocal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank your for your comment 🙇
using WindowWidthSizeClass directly here
I see 👍 I changed to use directly 🙏
c089579
I noticed an issue (#700). |
Now we are not using Lazy Colms for favorite items so implementing the animation might be difficult. But you don't have to think about the animation. |
Thank you for your comment 🙏 and I understood 🙇 In addition, I resolved conflicts! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for finding the way to do this and addressing points!
Issue
Overview (Required)
Links
Screenshot (Optional if screenshot test is present or unrelated to UI)
Movie (Optional)
Screen_recording_20240821_032012.mp4
Screen_recording_20240821_030209.mp4
Screen_recording_20240821_032105.mp4
Screen_recording_20240821_030629.mp4