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 UI issues for Sidebar #634

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

Conversation

abirc8010
Copy link
Contributor

@abirc8010 abirc8010 commented Oct 1, 2024

Brief Title

This PR fixes UI issues for Sidebar making it responsive

Acceptance Criteria fulfillment

  • Introduced a variable isSmallScreen to detect if the width of screen is <= 780px and adjusts the style of the component accordingly
  • For small screens , styled the Sidebar with position: absolute and covering entire width by setting width to 100%

Fixes # (issue)
#629

Video/Screenshots

Screencast.from.2024-10-02.02-07-54.webm

Popup Mode:

popup.webm

Note: The PR will be ready for live testing at https://rocketchat.github.io/EmbeddedChat/pulls/pr-634 after approval.

@abirc8010
Copy link
Contributor Author

@Spiral-Memory do I need to test it on some more cases ?

@Spiral-Memory
Copy link
Collaborator

Really loved the thorough testing done by you in the attached video, I'll review the PR and test myself once and let you know.

Thank you so much for your contribution ❤️

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Oct 1, 2024

Once go to design variants, modern design and check if popup mode is working fine after this change and attach a video

@abirc8010
Copy link
Contributor Author

I've attached the popup mode video ,
I haven't made any changes for the popup mode for now though , I introduced a conditional statement so that the styles are applied for the Sidebar component only

@abirc8010
Copy link
Contributor Author

Hey @Spiral-Memory , are any more tests required ?

@Spiral-Memory
Copy link
Collaborator

I haven't tested it yet, let the pr preview auto deployment feature get implemented, afterwards, I'll test and let you know if any changes are required

@Spiral-Memory
Copy link
Collaborator

Hey @abirc8010,

I noticed two issues. Please fix them:

  1. In normal mode, the sidebar title is properly visible, but when switching to a smaller screen (with less height), the title is not visible. Ensure that in both modes, the title aligns at the same height.

image
image

  1. While in dev mode, the switch does not happen automatically when changing screen sizes. I have to manually open and close the sidebar to reflect the changes, which is not the intended behavior.

@abirc8010
Copy link
Contributor Author

  1. While in dev mode, the switch does not happen automatically when changing screen sizes. I have to manually open and close the sidebar to reflect the changes, which is not the intended behavior.

To fix this issue, I have to add resize event listener , will that be ok ?

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Oct 6, 2024

  1. While in dev mode, the switch does not happen automatically when changing screen sizes. I have to manually open and close the sidebar to reflect the changes, which is not the intended behavior.

To fix this issue, I have to add resize event listener , will that be ok ?

But why do we need listeners ? Why can't we use media queries to handle it properly ?

@abirc8010
Copy link
Contributor Author

But why do we need listeners ? Why can't we use media queries to handle it properly ?

The Sidebar component accepts a style prop in the form of a css object, which makes it difficult to implement media queries effectively. That's why I am considering using a resize event listener instead .
Do you have any suggestions?

@Spiral-Memory
Copy link
Collaborator

But why do we need listeners ? Why can't we use media queries to handle it properly ?

The Sidebar component accepts a style prop in the form of a css object, which makes it difficult to implement media queries effectively. That's why I am considering using a resize event listener instead . Do you have any suggestions?

It is not recommended to use event listeners in this use case. Try using only media queries. You can still write the CSS in the style.js file and pass it. Try to figure out a solution using media queries.

@abirc8010
Copy link
Contributor Author

@Spiral-Memory , I have fixed the issues

Copy link
Collaborator

@Spiral-Memory Spiral-Memory left a comment

Choose a reason for hiding this comment

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

The approach seems fine; you are trying to add it to the global styles. However, why can't we add similar media queries and pass them directly to the sidebars, without overriding class styles globally?

@abirc8010
Copy link
Contributor Author

I noticed that the Sidebar component from the embeddedChat ui-element only accepts style prop as JavaScript objects, which doesn't allow for media queries. As a result, I had to apply the styles using the classname in global styles .

image

@Spiral-Memory
Copy link
Collaborator

As far as I'm aware, you can set media queries in a style object as well, or try using the css attribute with Emotion/React. I'll have to look into it to confirm.

Copy link
Collaborator

@Spiral-Memory Spiral-Memory left a comment

Choose a reason for hiding this comment

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

Approving to test !

@Spiral-Memory
Copy link
Collaborator

@devanshkansagra

Could you please check, if we can use media queries in style object ?.

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Oct 11, 2024

Unnecessary spacing !

Screenshot_2024-10-11-22-06-06-28_e4424258c8b8649f6e67d283a50a2cbc.jpg

@abirc8010
Copy link
Contributor Author

abirc8010 commented Oct 11, 2024

Hey @Spiral-Memory , I fixed the issue

Copy link
Collaborator

@Spiral-Memory Spiral-Memory left a comment

Choose a reason for hiding this comment

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

Approving to test !

@abirc8010
Copy link
Contributor Author

Approving to test !

@Spiral-Memory do I need to make any changes?

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.

3 participants