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

Refactor sidebar components #509

Merged
merged 5 commits into from
Mar 17, 2024

Conversation

JeffreytheCoder
Copy link
Contributor

@JeffreytheCoder JeffreytheCoder commented Mar 11, 2024

Brief Title

Acceptance Criteria fulfillment

  • All current menus share a general component and CSS module
  • The menus behave the same as before
  • The refactored menu framework have flexibility for all types of menus

Fixes #504

Video/Screenshots

All menus look the same as before

@JeffreytheCoder
Copy link
Contributor Author

Anyone knows why the lint check workflow fails with the following error?

>  Lerna (powered by Nx)   Running target format:check for 6 projects failed

   Tasks not run because their dependencies failed or --nx-bail=true:
   
   - e2e-react:format:check
   - @embeddedchat/htmlembed:format:check
   
   Failed tasks:
   
   - @embeddedchat/react:format:check

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Mar 11, 2024

Anyone knows why the lint check workflow fails with the following error?

>  Lerna (powered by Nx)   Running target format:check for 6 projects failed

   Tasks not run because their dependencies failed or --nx-bail=true:
   
   - e2e-react:format:check
   - @embeddedchat/htmlembed:format:check
   
   Failed tasks:
   
   - @embeddedchat/react:format:check

Format with prettier.

Run it in this file : src/components/Sidebar/Sidebar.module.css

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Mar 11, 2024

@JeffreytheCoder , This is an awesome change, great work bro!. It was difficult to achieve as well. Well, I suggest attaching a video showcasing everything working smoothly (all sidebar components) without any rendering issue or console errors for every functionality in those sidebars. I can see that you have only extracted the very common elements and passed the other part as a children prop. While it's not extremely modular, which is good as it reduces the chances of introducing bugs, as discussed earlier there is still potential for issues and since we have a plan to release it as next major version soon. Therefore, please ensure from your side as well, and including a video with no console errors will be helpful.

@JeffreytheCoder
Copy link
Contributor Author

@Spiral-Memory Thanks bro! It's hard to modularize sidebar content because they vary a lot, but having a modularized sidebar wrapper solves some inconsistent stylings, and it's better to maintain and add upcoming features like resizing along with the chat window.

Here's the recording below that shows all sidebars work as before, with no console warning or error:
sidebar-after

I'll create another PR to put pinned & starred messages into the general sidebar after #501 is reviewed.

@Spiral-Memory
Copy link
Collaborator

Spiral-Memory commented Mar 11, 2024

Yep, I told you, it has so much variation and is hard to modularize, hehe! Anyway, great work!

Yes, and until then, the file menu feature will also be added. Refactor both together.

Is the thread scrollbar working fine? Create many threads and check.

@JeffreytheCoder
Copy link
Contributor Author

Yes it works :)

many-threads

@Spiral-Memory
Copy link
Collaborator

Awesome 🥳🥳

@JeffreytheCoder
Copy link
Contributor Author

@sidmohanty11 Ready for review

Copy link
Collaborator

@sidmohanty11 sidmohanty11 left a comment

Choose a reason for hiding this comment

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

@JeffreytheCoder awesome! Had one question, how does it look in minimised mode?

@JeffreytheCoder
Copy link
Contributor Author

@sidmohanty11 Same as before, the sidebar expand to full screen:

sidebar-mobile

Copy link
Collaborator

@sidmohanty11 sidmohanty11 left a comment

Choose a reason for hiding this comment

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

Thanks @JeffreytheCoder !!

@sidmohanty11 sidmohanty11 merged commit bb3cdfa into RocketChat:develop Mar 17, 2024
3 checks passed
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.

Refactor menu components
3 participants