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

Fix white circle button is visible beneath the ChannelAvatarView #725

Merged

Conversation

sheep-q
Copy link
Contributor

@sheep-q sheep-q commented Jan 21, 2025

🔗 Issue Link

Github issue

🎯 Goal

The channel avatar view should show correctly without a visible white circle below.

🛠 Implementation

  • Change the style of button inside ToolbarItem
  • Update the initialization of ChannelAvatarView to remove deprecated init warninig.

🧪 Testing

🎨 Changes

image

☑️ Checklist

  • I have signed the Stream CLA (required)
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Affected documentation updated (docusaurus, tutorial, CMS (task created)

@@ -35,16 +35,13 @@ public struct DefaultChatChannelHeader: ToolbarContent {
}

public var channel: ChatChannel
public var headerImage: UIImage
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, we can't do any changes to the public API at the moment (only in major versions, but the next one is a minor upgrade). Could you please take these out and just keep the buttonStyle change.
Is there an additional issue with the headerImage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I deleted headerImage because when using it in the initialization of ChannelAvatarView, it shows warning init(avatar:showOnlineIndicator:size:)' is deprecated: Use automatically refreshing avatar initializer.
But no problem, it's not the main reason so I just keep the buttonStyle change

@sheep-q sheep-q force-pushed the fix/trailing-toolbarItem-button-style branch from 86d7021 to 1203442 Compare January 21, 2025 09:23
@@ -71,6 +71,7 @@ public struct DefaultChatChannelHeader: ToolbarContent {
.clipShape(Circle())
.offset(x: 8)
}
.buttonStyle(.plain)
Copy link
Contributor

Choose a reason for hiding this comment

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

I did some testing and seems like it also changes spacings:
Before:
Before
After:
After
I'll investigate this as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

What does not break the padding is changing the Button's offset from 8 -> 4.

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 think changing the Button's offset causes confusing. Clearly, this offset should be equal offset of ChannelAvatarView, either both set to 0 or 8.
I believe that, the issue was from SwiftUI framework that automatically add padding to ToolbarItem(placement: .navigationBarTrailing). I tested the same ZStack's content to several wrapped views without encountering any issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or you can just configure Button like this without multiple layers in ZStack, it doesn't care about additional padding.
I'm not sure about the purpose of your using separate layers, so I wouldn't use this approach.

               Button {
                    resignFirstResponder()
                    isActive = true
                } label: {
                    ChannelAvatarView(
                        avatar: headerImage,
                        showOnlineIndicator: onlineIndicatorShown,
                        size: CGSize(width: 36, height: 36)
                    )
                    .offset(x: 8)
                }

Copy link
Contributor

Choose a reason for hiding this comment

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

Our default design has custom spacing defined for the avatar shown in the toolbar in relative to the messages in the message list. This is why the offset is used there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @laevandus, Would you like to change the offset for both Button and ChannelAvatarView or just for Button

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which solution did you approve?
Wrap the ChannelAvatarView inside the Button or modify the Button style (current change)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @sheep-q, let's go with what you proposed before, but with offset 4 (our default design's requirement)

Button {
                    resignFirstResponder()
                    isActive = true
                } label: {
                    ChannelAvatarView(
                        avatar: headerImage,
                        showOnlineIndicator: onlineIndicatorShown,
                        size: CGSize(width: 36, height: 36)
                    )
                    .offset(x: 4)
                }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @laevandus, I've pushed an updated commit, take a look. thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @sheep-q I'll take it in soon.

@sheep-q sheep-q force-pushed the fix/trailing-toolbarItem-button-style branch from 2d8382b to 5f1afcb Compare January 24, 2025 13:54
@sheep-q sheep-q force-pushed the fix/trailing-toolbarItem-button-style branch from 5f1afcb to 24a25e9 Compare January 24, 2025 14:03
Copy link
Contributor

@laevandus laevandus left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! ✅

@laevandus laevandus enabled auto-merge (squash) January 27, 2025 08:29
@laevandus laevandus disabled auto-merge January 27, 2025 08:29
@laevandus
Copy link
Contributor

I will add a CHANGELOG entry in a separate PR. Also verified that this change does not create any snapshot test failures.

@laevandus laevandus merged commit 8d760b4 into GetStream:develop Jan 27, 2025
5 of 9 checks passed
@Stream-SDK-Bot Stream-SDK-Bot mentioned this pull request Jan 28, 2025
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.

2 participants