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

Fixed message box misalignment issue #619

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from

Conversation

abirc8010
Copy link
Contributor

@abirc8010 abirc8010 commented Sep 2, 2024

Brief Title

  1. Increased the chat body’s max-height from 600px to 100%.

  2. Set the message box’s margin-bottom to 0 and margin-top to auto for better alignment at the
    bottom.

  3. Updated the chat input box margin to "2rem 2rem 0.4rem 1.5rem" for improved spacing and layout.

  4. Added media queries to adjust message font sizes for screen widths <= 900px , <= 600px, and <=
    400px to maintain readability.

Acceptance Criteria fulfillment

  • Task 1
  • Task 2
  • Task 3

Fixes #616

Video/Screenshots

Before:

before.webm

After:

after.webm

@CLAassistant
Copy link

CLAassistant commented Sep 2, 2024

CLA assistant check
All committers have signed the CLA.

@Spiral-Memory
Copy link
Collaborator

Hey @abirc8010

Thank you for your PR !
Please make a complete video showing that it works properly in all screen sizes, also highlight the changes you have done properly in PR description.

@abirc8010
Copy link
Contributor Author

@Spiral-Memory Thanks for the feedback. Currently, the responsiveness is for screens with a width of 400px or more. Can I work on improving it for screens narrower than 400px as well?

@Spiral-Memory
Copy link
Collaborator

Once, make a video for all screen sizes, and let me have a look at how it behaves. Based on that, I'll suggest you to work on it further.

Please update your PR description and video !

@abirc8010
Copy link
Contributor Author

I have updated my PR description and video

@Spiral-Memory
Copy link
Collaborator

Video looks good to me !

Regarding PR description, please add technical details on what you did as well ! It will be helpful for me to review as these are mostly css changes.

@abirc8010
Copy link
Contributor Author

Thanks I'm glad to know that it looks good !
I've updated the PR description as well !

@abirc8010
Copy link
Contributor Author

@Spiral-Memory do I need to make any further improvements in this PR ?

@Spiral-Memory
Copy link
Collaborator

Will review it, then I'll respond to it

@abirc8010
Copy link
Contributor Author

@Spiral-Memory is there any update on this PR ?
I've resolved the merge conflicts.

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Sep 29, 2024

Hey @abirc8010,
Please attach a 'before' video as well, so it will be helpful for me to review. (Record your own video of the current develop branch and compare it with your changes.)

Also, the build check is failing. Could you please format your changes using Prettier?

@abirc8010
Copy link
Contributor Author

@Spiral-Memory I’ve made the required changes

@Spiral-Memory
Copy link
Collaborator

Thank you so much for the contribution.

I need to verify this on multiple devices because, on my system, even before this PR, it looks the same as what is shown in the 'After' video 😢.

I'll let you know.

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Sep 29, 2024

@devanshkansagra

Could you please attach a before / after video for this PR as well recorded from your device.

Thanks..

@abirc8010
Copy link
Contributor Author

I need to verify this on multiple devices because, on my system, even before this PR, it looks the same as what is shown in the 'After' video 😢.

Before this PR , the issue is arising when the length of the screen is increased

Screencast.from.2024-09-29.17-10-33.webm

@Spiral-Memory
Copy link
Collaborator

Ohh, Okay
Thanks for the clarification, so in normal settings, this is fine right ?

@devanshkansagra
Copy link
Contributor

image
on my iphone

@@ -9,7 +9,7 @@ export const getChatbodyStyles = () => {
overflow-x: hidden;
display: flex;
flex-direction: column-reverse;
max-height: 600px;
max-height: 900px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, even after this PR, when this particular element exceeds the limit, the behavior will still be the same, right?

Can’t we consider using percentages or relative units instead of fixed units?

Because even with this change, there doesn't seem to be any real benefit.

What’s your opinion on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok , I am changing it to 100%

@Spiral-Memory
Copy link
Collaborator

image
on my iphone

This looks good except for the Quote messages.. anyway, I've opened an issue to make EmbeddedChat properly mobile responsive. It would be great if you could identify and address the various responsiveness issues, if you're interested. It's a large task, so collaborative work might be helpful!

@devanshkansagra @abirc8010 or anyone else who is interested.

@Spiral-Memory
Copy link
Collaborator

#629

@abirc8010
Copy link
Contributor Author

@Spiral-Memory Could I create a separate PR for the quote messages and mobile responsiveness? There are existing issues with quote message responsiveness that were present even before this PR, so those can also be addressed in that new PR.

@Spiral-Memory
Copy link
Collaborator

Keep the responsiveness issue as the central issue and raise PRs for small fixes quoting that issue, but significant changes that add real value are appreciated.

@abirc8010
Copy link
Contributor Author

I've also modified the max height to 100% as suggested

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.

Message Box Misalignment in Large Box Viewport
4 participants