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

Add support for overriding bubbleMargin using ChatTheme #585

Merged
merged 3 commits into from
Apr 25, 2024

Conversation

ishchhabra
Copy link
Contributor

What does it do?

This change adds support for overriding the bubbleMargin property using ChatTheme.

Why is it needed?

This allows users of the package to customize the margin of the chat bubbles, enabling scenarios where the user wants the message to take the entire width of the screen.

@qeepcologne
Copy link

very similar to what i did. Sorry i did not see your work, because it does not mention the corresponding task #496

@ishchhabra
Copy link
Contributor Author

very similar to what i did. Sorry i did not see your work, because it does not mention the corresponding task #496

My bad, I should've mentioned the open issue in my PR to avoid duplicated work. I think, though, the ability to completely override the entire margin offers higher flexibility and could be a better approach. What do you think @qeepcologne?

@qeepcologne
Copy link

I think you have no access to bubbleRtlAlignment in ChatTheme. If you want the same behaviour just with less padding how to do that?

@ishchhabra
Copy link
Contributor Author

I think you have no access to bubbleRtlAlignment in ChatTheme. If you want the same behaviour just with less padding how to do that?

Since bubbleRightAlignment is a property that is provided by the user here

this.bubbleRtlAlignment = BubbleRtlAlignment.right,

I think the user can simply access the value that they are passing to Chat. If a user is overriding the base functionality, I think it is reasonable to ask the user to completely handle that themselves. Alternatively, to achieve what I think you are trying for (to only modify the left padding), bubbleMargin could be modified to rather be EdgeInsets Function({required EdgeInsets defaultBubbleMargin}) or the package can simply expose getDefaultBubbleMargin as a util function. I prefer the latter as that still keeps the functionality as simple as possible.

@qeepcologne
Copy link

qeepcologne commented Apr 10, 2024

EdgeInsets is well known, clear and flexible.

But if you want to use the full width you have to do some caculations:
e.g.:
messageWidthRatio: 1 - 0.06 - 2 * 0.02
when the horizontal padding on left+right is MediaQuery.of(context).size.width * 0.02
and avatars are not used.
But thats another topic.

@ishchhabra
Copy link
Contributor Author

EdgeInsets is well known, clear and flexible.

But if you want to use the full width you have to do some caculations: e.g.: messageWidthRatio: 1 - 0.06 - 2 * 0.02 when the horizontal padding on left+right is MediaQuery.of(context).size.width * 0.02 and avatars are not used. But thats another topic.

Yep, for now, I think the current implementation should be sufficient. Let me know if you want me to update this PR to also export a getDefaultBubbleMargin function or change bubbleMargin to a function which takes defaultBubbleMargin as parameter to consolidate both our PRs into one.

@qeepcologne
Copy link

qeepcologne commented Apr 16, 2024

Sorry for the late reply, i tried to switch to your branch today:

  • can we change bubbleMargin to EdgeInsetsGeometry to allow EdgeInsetsDirectional also?

@ishchhabra
Copy link
Contributor Author

Sorry for the late reply, i tried to switch to your branch today:

  • can we change bubbleMargin to EdgeInsetsGeometry to allow EdgeInsetsDirectional also?

Great catch. Updated to use EdgeInsetsGeometry.

@ishchhabra
Copy link
Contributor Author

@demchenkoalex Could you please review this pull request? Are there any additional changes required before it can be merged?

@demchenkoalex demchenkoalex merged commit 2b5978c into flyerhq:main Apr 25, 2024
2 checks passed
@demchenkoalex
Copy link
Member

thanks for the PR :)

@theprojectfactory
Copy link

Hi @demchenkoalex,

Sorry for the ping.

When can we get this released? Our app is needing this exact feature

@ishchhabra ishchhabra deleted the feat/bubble-margin branch May 12, 2024 20:22
@ishchhabra
Copy link
Contributor Author

@theprojectfactory In the mean time until there is a new release, you can also directly target the exact code you want using

flutter_chat_ui:
    git: 
      url: ...
      ref: ...

@demchenkoalex
Copy link
Member

just came back from PTO, will release this week

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.

4 participants