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

feat: 🌟 use column instead of stack to remove arbitrary paddings #196

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jonasbadstuebner
Copy link
Contributor

@jonasbadstuebner jonasbadstuebner commented Jun 17, 2024

Description

Unless you have had a reason to make the setup stacked instead of a column in the first place, we can stop guessing the bottom inset for the chat message list now.
I don't like that you hardcoded the 100-height SizedBox.
Also I added the padding to the top of the Textfield, so we don't have to remember to put a bottom padding in all the other places.

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Related Issues

part of #187

@vatsaltanna-simformsolutions
Copy link
Collaborator

We can't use a column due to the UI requirements. I have shared a screenshot for reference. Additionally, the 100px static height was a mistake made during debugging. I will update this in a separate PR.

image

@jonasbadstuebner
Copy link
Contributor Author

The screenshot you showed me is using a Stack I guess.

@jonasbadstuebner
Copy link
Contributor Author

Talking about this Stack:

You are putting/rendering the SendMessageWidget


...on top of e.g. the ChatListWidget...
return ChatListWidget(

...only to push the latter out from underneath it with a SizedBox.

There is no reason to do that.
Also this part does not have to come back after merging this PR:

height: (MediaQuery.of(context).size.width *
(widget.replyMessage.message.isNotEmpty ? 0.3 : 0.14)) +
(widget.chatTextFieldTopPadding),

Because the TextField will push the chat further up when it grows due to a reply being initiated.

@vatsaltanna-simformsolutions
Copy link
Collaborator

I have shared a screenshot for your branch only.

@vatsaltanna-simformsolutions
Copy link
Collaborator

check this PR. #197

@jonasbadstuebner
Copy link
Contributor Author

Are you talking about the padding above the text field? 😅

@jonasbadstuebner
Copy link
Contributor Author

Or do you want to have the messages be shown behind the text field?

@vatsaltanna-simformsolutions
Copy link
Collaborator

vatsaltanna-simformsolutions commented Jun 18, 2024

Yes, it should be displayed behind the field, primarily behind the top corners of the text field. I initially tried using a column but then realized the reason for using a stack and reverted it back to a stack. 😅

@jonasbadstuebner
Copy link
Contributor Author

I'm offering a better solution (and fixing a weird thing where I did not see any error or anything due to the config not having settings that would show anything).

@jonasbadstuebner
Copy link
Contributor Author

I could make the overflow smaller, but it is working.

@jonasbadstuebner
Copy link
Contributor Author

Please test my changes again and let me know what you think.

@aditya-css
Copy link
Collaborator

Hello @jonasbadstuebner,
Thanks for this PR. Nice Work.

I tested your code and have found following two things:
Screenshot 2024-06-19 at 6 47 58 PM

  1. In the screenshot attached you can see that the texts are visible below the textfield as well. This is not desired. For reference, you can check the chat screen of Instagram and see that the texts are visible till the curve of the textfield only and not in the area below it.
  2. The scrollbar on the right in the attached screenshot is quite long and misleading from user perspective. This seems primarily because of using MediaQuery.of(context).size.height in chat_groupedlist_widget.dart and chat_view.dart files in your changes.

Can you please address above mentioned changes and update your PR?

Copy link
Collaborator

@aditya-css aditya-css left a comment

Choose a reason for hiding this comment

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

The issue 2 from previously mentioned issues is resolved, but the first issue of texts appearing below the textfield still exists.

This is the current version from this PR:
https://github.com/SimformSolutionsPvtLtd/flutter_chatview/assets/98031138/bc564f35-088d-4e41-bf92-057ff0ece64c

This is what we desire:
https://github.com/SimformSolutionsPvtLtd/flutter_chatview/assets/98031138/a75c44b0-fab6-4249-8cef-125f302a1395

lib/src/widgets/chat_groupedlist_widget.dart Show resolved Hide resolved
clipBehavior: Clip.none,
children: [
Positioned.fill(
bottom: -MediaQuery.of(context).size.height,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally, this should be as big as the textfield is and not some large value like screen height.

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 cannot know how large the Textfield is, because the Textfield is built after the chat list.

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 found a different solution, let me know what you think after my next push.

lib/src/widgets/custom_scroll_behavior.dart Show resolved Hide resolved
@override
Widget buildScrollbar(
BuildContext context, Widget child, ScrollableDetails details) {
switch (getPlatform(context)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please convert this to switch expression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the way it is implemented in the parent class. I don't think the switch expression would help here, considering I would have to be very verbose.

@@ -0,0 +1,32 @@
import 'package:flutter/material.dart';

class CustomScrollBehavior extends ScrollBehavior {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe a more specific name would make more sense than just using custom as prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a recommendation?
BottomPaddedScrollBehavior?

lib/src/widgets/chat_view.dart Show resolved Hide resolved
lib/src/widgets/chat_view.dart Show resolved Hide resolved
lib/src/widgets/chat_list_widget.dart Show resolved Hide resolved
lib/src/widgets/chat_groupedlist_widget.dart Show resolved Hide resolved
lib/src/widgets/chat_groupedlist_widget.dart Show resolved Hide resolved
@jonasbadstuebner
Copy link
Contributor Author

Yes, I am not done with the implementation yet. But I will address your comments later.

@Gaurav-CareMonitor
Copy link

@jonasbadstuebner any progess?

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