DroidKaigi / conference-app-2024

The Official Conference App for DroidKaigi 2024
Apache License 2.0
421 stars 200 forks source link

On devices with a small width, the shared image in the profile card will be something unintended. #890

Closed Corvus400 closed 1 week ago

Corvus400 commented 2 weeks ago
SmallPhone Portrait SmallPhone Landscape

https://github.com/DroidKaigi/conference-app-2024/blob/53f341822fd0f4a128db83648e805d75156c7f2a/feature/profilecard/src/commonMain/kotlin/io/github/droidkaigi/confsched/profilecard/component/ShareableCard.kt#L94

Corvus400 commented 2 weeks ago

I've tried it out, and it looks like it might be possible to output a fixed size on any device, so I'll try it myself.

mannodermaus commented 2 weeks ago

Great catch. πŸ‘€

Since this composable is not actually displayed on screen, I wonder if we can force it into a more consistent layout by giving it a specific size? In other words, could it work if we assigned size(1200.dp, 630.dp) to ShareableCardContent's modifier?

edit: ι…γ™γŽγŸγ€œ πŸ˜†

Corvus400 commented 2 weeks ago

@mannodermaus I'm sorry. πŸ™‡ I've made a PR, so if you could try outputting it in portrait and landscape on your own device and check for any design issues, that would be very helpful. πŸ™‡

mannodermaus commented 2 weeks ago

No problem, I will check it in a moment! At first glance, your PR looks great and should work, though. Good job!

mannodermaus commented 2 weeks ago

γŠεΎ…γŸγ›γ—γΎγ—γŸγ€‚The layout seems to work pretty well and looks identical, no matter if I export it portrait or landscape – nice. I was able to create a change when changing the font size and display size to the maximum, though. The landscape version looks a bit odd with this and changes its shape. This might be negligible, though.

Portrait Landscape
Normal font
Large font
Corvus400 commented 1 week ago

@mannodermaus If we handle corner cases in the current PR, it will be difficult to review, so I would like to create a new issue and handle it there. πŸ™‡ (The issue itself will be created once the PR we are currently submitting has been merged.) I'm sorry. πŸ™

mannodermaus commented 1 week ago

I agree that keeping it separate is the way to go. Let's get this improvement in first and then we can define the next one. 😊

Corvus400 commented 1 week ago

@mannodermaus Thank you for waiting. πŸ™‡ We have created a new PR, and have addressed the problem of the display breaking up when the font or display scale is made smaller or larger. πŸ™