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

Adjusted the sidebar width to cover the entire width for small screens #630

Merged

Conversation

abirc8010
Copy link
Contributor

@abirc8010 abirc8010 commented Sep 30, 2024

Brief Title

This PR ensures the sidebar behaves responsively by making it occupy the full screen on devices with a screen width of 780px or smaller. For larger screens, the sidebar retains its original layout.

Acceptance Criteria fulfillment

  • For small screens (≤ 780px), the sidebar is given position: 'absolute' and a full width of 100%vw

Fixes # (issue)
#629

Video/Screenshots

Before:

Before.webm

After:

Screencast.from.2024-10-01.00-44-23.webm

@abirc8010
Copy link
Contributor Author

@Spiral-Memory I have made the Sidebar responsive

@@ -75,7 +75,7 @@ export const getMessageDividerStyles = (theme) => {
line-height: 1rem;
position: relative;
display: flex;
z-index: 1000;
z-index: 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any specific reason for changing this? The current structure uses a specific set of z-index values for different elements, so it behaves correctly during integration. Is it necessary to change it? Please look for ways to avoid this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The z-Index is made 1 so that the date for the messages does not appear over sidebar menu when it gets adjusted to Full Screen

setIsSmallScreen(window.innerWidth <= 780);
};

window.addEventListener('resize', handleResize);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest using media queries instead. In my opinion, a resize event listener is not needed at all. Consider how mobile devices work—will they be resizing their screens?

return () => window.removeEventListener('resize', handleResize);
}, []);

const viewComponentStyle = isSmallScreen
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't define styles here. There is nothing wrong with it, but we follow a specific structure where all major styles are in the component.style.js file, and only inline styles should be applied in the component.js file. Look at other components and please follow the same pattern.

setIsSmallScreen(window.innerWidth <= 780);
};

window.addEventListener('resize', handleResize);
Copy link
Collaborator

@Spiral-Memory Spiral-Memory Sep 30, 2024

Choose a reason for hiding this comment

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

Same comment as above !

return () => window.removeEventListener('resize', handleResize);
}, []);

const viewComponentStyle = isSmallScreen
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above !

setIsSmallScreen(window.innerWidth <= 780);
};

window.addEventListener('resize', handleResize);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above !

@Spiral-Memory
Copy link
Collaborator

Thank you so much, @abirc8010 !
The video looks great and is awesome.

However, I’ve suggested a few changes. Please go through them.

Thanks!
Happy contributing

@abirc8010
Copy link
Contributor Author

@Spiral-Memory then can I change these styles to inline styles ?

@Spiral-Memory
Copy link
Collaborator

Nope, there are many styles that you have applied for. Please follow the 'component.styles.js' approach like in other components to keep things consistent.

@abirc8010
Copy link
Contributor Author

@Spiral-Memory, I have
reverted the z-index value to 1000 ,
used media queries to adjust the width of sidebar
removed the resize event listener

@Spiral-Memory
Copy link
Collaborator

Looks good to me on an initial review, please update your 'after' videos showcasing how it behaves now after the change. I'll review it thoroughly and let you know

Awesome work 👏🎉 @abirc8010

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Sep 30, 2024

Hey @abirc8010

Please fix this lint issue and make sure you've formatted everything with prettier:

error Expected 1 empty line after import statement not followed by another import

@abirc8010
Copy link
Contributor Author

@Spiral-Memory I have fixed the linting issue and updated the after video

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Sep 30, 2024

In the video, you haven't shown, whether it behaves like before in normal screen sizes.

@abirc8010
Copy link
Contributor Author

abirc8010 commented Sep 30, 2024

In the video, you haven't shown, is it behaving like before in normal screen sizes.

I've updated it now

@Spiral-Memory Spiral-Memory merged commit 0aafed8 into RocketChat:develop Sep 30, 2024
3 checks passed
@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Oct 1, 2024

Hey @abirc8010 ,
There are multiple issues after this merge. I would advise you to fix them as soon as possible; otherwise, I will need to revert this PR.

Some issues include:

  1. After creating multiple threads, the scroll functionality is not working, this is true for all sidebars, after the content exceeds height, the scroll within the sidebar is not working.
  2. On smaller screens, the content is not aligned properly in all sidebars.
    image
  3. On regular screens, the "Not found" messages are not center-aligned in all sidebars.
    image
  4. On smaller screen, in members, while the members are fetching, it is not taking the full space and takes only when it is loaded.

These are the issues I noticed after the PR was deployed, but there could be more. Please address them and thoroughly test PRs under multiple conditions to prevent such issues from recurring.

Thanks for your help.

@abirc8010
Copy link
Contributor Author

@Spiral-Memory

About the alignment , I think it can be better managed , if I can apply the inline styling to Viewcomponent and maintain a isSmallScreen variable

For rest of the issues, I will address them by today

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Oct 1, 2024

Another issue;
image

Also it has disturbed the popup mode..

I'm reverting this PR.. You can raise a different PR addressing all the concerns.
Thanks

@abirc8010
Copy link
Contributor Author

@Spiral-Memory Could you please tell me why a high value of z-index is chosen here :

image

and how to reproduce this :

image

@Spiral-Memory
Copy link
Collaborator

@Spiral-Memory Could you please tell me why a high value of z-index is chosen here :

image

and how to reproduce this :

image

The zIndex values have been in place since the start of the project. Changing one value can disrupt the project in multiple areas. To prevent this, I have designed other elements with respect to these values, ensuring nothing is affected. It is recommended to follow this pattern. However, if you do change it, you must thoroughly test everything under all possible conditions to ensure it works as expected.

To reproduce the issue, open the sidebar on a standard laptop screen and hover over the message.

@abirc8010 abirc8010 deleted the fix/sidebar-responsiveness branch October 2, 2024 12:20
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