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

♻️ [ShareableCardContent] Display a shadow instead of a border around the profile picture. #906

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

Conversation

Corvus400
Copy link
Contributor

@Corvus400 Corvus400 commented Aug 31, 2024

Issue

Overview (Required)

  • We solved the problem of not being able to display the shadow, which had been an issue with the OGP sharing function, and now the shadow is displayed instead of the border.

Links

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

Before After

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 31, 2024 21:29 Inactive
Copy link

Snapshot diff report

File name Image
ShareableCardPreview
_compare.png

@Corvus400 Corvus400 changed the title [ShareableCardContent] ♻️ Display a shadow instead of a border around the profile picture. ♻️ [ShareableCardContent] Display a shadow instead of a border around the profile picture. Aug 31, 2024
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 1, 2024 05:13 Inactive
@Corvus400 Corvus400 marked this pull request as ready for review September 1, 2024 05:26
// A larger number results in a more diffused and smoother shadow.
val maxOffset = 20f

for (i in 1..shadowLayers) {
Copy link
Member

@takahirom takahirom Sep 1, 2024

Choose a reason for hiding this comment

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

It might be difficult but, how about using Brush as the card surface effect?
https://developer.android.com/develop/ui/compose/graphics/draw/brush

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
I tried it for a few hours, but unfortunately I couldn't get the same expression as the current implementation. 🥺

Copy link
Member

Choose a reason for hiding this comment

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

And this you might tried but how about just using .shadow(elevation= )?

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
Unfortunately, that method didn't work either. 😞
Incidentally, I tried various things afterwards, such as surrounding it with a card to see if the elevation would be displayed, but the elevation is ignored in the same way as the shadow. 💀

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
Thank you for waiting. 🙇
I tried using the Brush method.
I think it's simpler and easier to understand than the original processing, but what do you think?
It's a little different from the original design... but if you don't want to make it too complicated, I think this is a good compromise. 🙏

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 3, 2024 00:29 Inactive
…design/display_a_shadow_instead_of_a_line

# Conflicts:
#	feature/profilecard/src/commonMain/kotlin/io/github/droidkaigi/confsched/profilecard/component/CaptureableCardBack.kt
#	feature/profilecard/src/commonMain/kotlin/io/github/droidkaigi/confsched/profilecard/component/CaptureableCardFront.kt
#	feature/profilecard/src/commonMain/kotlin/io/github/droidkaigi/confsched/profilecard/component/ShareableCard.kt
Copy link

github-actions bot commented Sep 4, 2024

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

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 4, 2024 04:37 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 4, 2024 04:58 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 5, 2024 01:00 Inactive
Comment on lines +171 to +181
rotate(degrees = 180f) {
for (i in 1..15) {
drawRoundRect(
brush = sweepGradient,
size = Size(cardWidthPx.toFloat(), cardHeightPx.toFloat()),
topLeft = Offset(-i.toFloat(), -i.toFloat()),
cornerRadius = CornerRadius(with(density) { 16.toDp().toPx() }),
)
}
}
drawImage(imageBitmap)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sweepGradient can only paint colors from the 3 o'clock direction.
So, I rotated it 180 degrees and drew the image on top of that.

Comment on lines +172 to +173
for (i in 1..15) {
drawRoundRect(
Copy link
Contributor Author

@Corvus400 Corvus400 Sep 5, 2024

Choose a reason for hiding this comment

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

It was impossible to make a shadow like Figma with just one brush, no matter how hard I tried.
So I think the only way is to draw it like this, gradually shifting the offset. 🙏

cardHeightPx: Int,
modifier: Modifier = Modifier,
) {
val sweepGradient = Brush.sweepGradient(
Copy link
Contributor Author

@Corvus400 Corvus400 Sep 5, 2024

Choose a reason for hiding this comment

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

The only brush that could create a shadowy effect like Figma was the sweepGradient.
RadialGradient is good for drawing perfect circles, but it is not suitable for drawing shadows on rectangular cards like this one.
No matter how you try, it's impossible to achieve the same kind of rendering as Figma. 🙏

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 6, 2024 06:53 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 6, 2024 23:47 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 7, 2024 01:21 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 7, 2024 04:20 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 7, 2024 10:34 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 7, 2024 13:01 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 7, 2024 13:47 Inactive
@github-actions github-actions bot temporarily deployed to deploygate-distribution September 7, 2024 14:38 Inactive
@Corvus400
Copy link
Contributor Author

@takahirom
I'm sorry, but I'm not sure whether I should wait for a review or whether I need to take any additional action, so please tell me. 🙏
This PR should be ready to be reviewed. 🙇

@takahirom
Copy link
Member

takahirom commented Sep 8, 2024

The current status is that I want to explore the possibility of improving the multiple calls to drawRoundRect with debugging. However, I have a lot to do.

@Corvus400
Copy link
Contributor Author

@takahirom
I understand. 🫡
I will make sure that drawRoundRect is only executed once.
To get closer to the design, would you accept a slightly more complex process?
Or would you prefer to keep the processing simple while still getting close to the design?

@takahirom
Copy link
Member

I think so.
And I haven't tried the shadows myself, so I'm not really sure why they don't work.
https://developer.android.com/develop/ui/compose/graphics/draw/shapes#use-polygon

@Corvus400
Copy link
Contributor Author

@takahirom
I see...
I'll try using polygons too.
I'd like to check again, but is it better to make the processing more complex and get closer to the design?
Or do you want to avoid making the processing more complex and stick to the design as much as possible?
Ideally, it would be best if the processing was easy to understand and followed the design, but considering that it hasn't worked well so far, it is likely that we will need to compromise on one or the other.

@takahirom
Copy link
Member

I think it depends on how complex it is, so I can't say for sure. I just want to see if we can improve it, and it's not your fault.

@takahirom
Copy link
Member

The elevation is just a test, but I'm not sure why we need such a complex process for this. 🙇
image

@takahirom
Copy link
Member

Oh, I see. When we actually share it, the shadow doesn't appear correctly, right?

@takahirom
Copy link
Member

@Corvus400
Copy link
Contributor Author

@takahirom

When we actually share it, the shadow doesn't appear correctly, right?

I see, so this part wasn't getting across, so the PR review wasn't progressing.

https://kotlinlang.slack.com/archives/CJLTWPH7S/p1712209788075759?thread_ts=1712186047.137109&cid=CJLTWPH7S

Unfortunately, I can't see the Slack link because there is no way to join that workspace. 😢
If you need this for your implementation, I would appreciate it if you could share the information with me. 🙇

@takahirom
Copy link
Member

takahirom commented Sep 8, 2024

You can view this and join Slack using the button at the top right:
https://slack-chats.kotlinlang.org/t/16903564/in-the-recent-jetpack-compose-update-i-discovered-an-intrigu#4c753cad-e7ad-47e5-acde-9268b00d6bcf

And I want to try this implementation. Once it works, we can't copy and paste so we have to rewrite it though.
https://slack-chats.kotlinlang.org/t/16319671/i-m-using-a-graphics-layer-and-off-screen-compositing-to-dra#b157b8e4-52fe-43cf-8f7d-999699a7cac2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants