RocketChat / EmbeddedChat

An easy to use full-stack component (ReactJS) embedding Rocket.Chat into your webapp
https://www.npmjs.com/package/@embeddedchat/react
107 stars 214 forks source link

chore: minor bugs #506

Closed Spiral-Memory closed 3 months ago

Spiral-Memory commented 4 months ago

Brief Title

This is just a small PR to fix some of the minor bugs i could see in the console and in appearance

Acceptance Criteria fulfillment

Fixes # NA

I am not creating an issue for this as it is not a very significant PR, it just fixes some of console errors, button appearance and some linting warnings and unnecessary renders.

Attaching some screenshots of issues which i tried fixing here:

image

Screenshot from 2024-03-09 21-20-22

image

image

Spiral-Memory commented 4 months ago

Hey @umangutkarsh , take a look here. In this video, everything works fine; your recent message button functions perfectly. However, when we make changes to the code, and the page automatically re-renders, the recent message functionality doesn't work and also throws console error and can't update the state as component has been unmounted. One has to refresh the page to make it work again.

I know it's not a crucial issue because re-rendering due to code changes won't happen in real-time and is only during development. Still, if you could look into it, it would be great. There were other console errors as well, such as not being able to find scrollTop and being unable to remove event listeners, among others, these were there because messageRefList.current is not available as component is not there and there were no null checking. I tried fixing those, but the root reason is unknown as to why the component doesn't mount again. Since you have written it, i feel you might be better able to look into this.. whenever you are free as this issue is not significant.

https://github.com/RocketChat/EmbeddedChat/assets/78961432/fa8ee97c-a7e2-4e45-9127-142e6af5d84b

Spiral-Memory commented 4 months ago

@sidmohanty11 Just a very small PR fixing basic issues. Have a look whenever you are free.

umangutkarsh commented 3 months ago

Hey @umangutkarsh , take a look here. In this video, everything works fine; your recent message button functions perfectly. However, when we make changes to the code, and the page automatically re-renders, the recent message functionality doesn't work and also throws console error and can't update the state as component has been unmounted. One has to refresh the page to make it work again.

I know it's not a crucial issue because re-rendering due to code changes won't happen in real-time and is only during development. Still, if you could look into it, it would be great. There were other console errors as well, such as not being able to find scrollTop and being unable to remove event listeners, among others, these were there because messageRefList.current is not available as component is not there and there were no null checking. I tried fixing those, but the root reason is unknown as to why the component doesn't mount again. Since you have written it, i feel you might be better able to look into this.. whenever you are free as this issue is not significant.

2024-03-10.01-59-50.mp4

Hi @Spiral-Memory . Thanks for the PR and thanks for the info. I tried to fix the issues related to that 'new-message-notification' feature few weeks back. I feel the issues are majorly due to the re-rendering of the page whenever any action on the page is performed. Will definitely look into it once I have some spare time.

Thanks for letting me know.

Spiral-Memory commented 3 months ago

Hey @umangutkarsh Yep! I see that you've already resolved many things with respect to this feature, including adding the otherUserMessage and other elements.

It's not very important issue; whenever you're free, take a look at it. It only happens when the page renders after the rebuild, not during normal re-renders. That's why it's not a big issue. No worries.