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

wip: inappchat component for virtual conference #78

Merged
merged 9 commits into from
Feb 21, 2022

Conversation

sidmohanty11
Copy link
Contributor

@sidmohanty11 sidmohanty11 commented Feb 10, 2022

rocketchatcomponent1.mp4

#38

@sidmohanty11
Copy link
Contributor Author

Fetching Messages And Sending Messages

rocketchatcomponent2.mp4

@lgtm-com
Copy link

lgtm-com bot commented Feb 10, 2022

This pull request introduces 1 alert when merging c9b9cc0 into 1386a00 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Feb 11, 2022

This pull request introduces 1 alert and fixes 1 when merging 203361d into 1386a00 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

fixed alerts:

  • 1 for Unused variable, import, function or class

@Sing-Li Sing-Li changed the title wip: rocket chat component wip: inappchat component for virtual conference Feb 12, 2022
Copy link
Member

@Sing-Li Sing-Li left a comment

Choose a reason for hiding this comment

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

First review comments.

app/components/clientsideonly/fuselage.js Outdated Show resolved Hide resolved
app/components/rocketchat.js Outdated Show resolved Hide resolved
app/lib/rocketchatapi.js Outdated Show resolved Hide resolved
app/package-lock.json Outdated Show resolved Hide resolved
app/styles/RocketChat.module.css Outdated Show resolved Hide resolved
@@ -9,6 +9,10 @@
"lint": "next lint"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can move these down to the componenet level -- making each one of our components an indepdendently installable npm module (optionally).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we can achieve this by making a monorepo and optionally dealing with components as we need. I will talk with Rohan sir about this and let you know

@Sing-Li
Copy link
Member

Sing-Li commented Feb 12, 2022

rocketchatcomponent1.mp4
#38

This is terrible UX fwiw. When there is no chat - we want the video to be in "full size" maxed out on the frame without sacrificing aspect. The "tab" or "button" or "arrow" to activate chat should be up at the top right - as small as reasonable -- and be transparent if placed over the playing video. On mobile, there should be no "tab"/"button"/"arrow" at all, but just a "swipe right" gesture.
In no case should be there be "jumbo white space" and "jumbo blue button" that hogs real estate for no purpose at all.

Please confer with irfan on the "design" for next iteration.

@sidmohanty11
Copy link
Contributor Author

True, I've just placed them as placeholders for now.Will be working on them soon

@lgtm-com
Copy link

lgtm-com bot commented Feb 15, 2022

This pull request introduces 2 alerts and fixes 1 when merging 691ad32 into 3927e64 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

fixed alerts:

  • 1 for Unused variable, import, function or class

@Sing-Li
Copy link
Member

Sing-Li commented Feb 18, 2022

@sidmohanty11 does the current code include the emoji support you have shown us today?

I'd like to marge it down ASAP so @demonicirfan can add the animations.

Let me know. It would be good if the cookie implementation can be added as well (but not necessarily at this time).

@sidmohanty11
Copy link
Contributor Author

Just give me one more day so I can try my best to figure out if we can use the cookies and clean some things up

Thank you!

@lgtm-com
Copy link

lgtm-com bot commented Feb 19, 2022

This pull request fixes 1 alert when merging 4b79c36 into 3927e64 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@sidmohanty11
Copy link
Contributor Author

@Sing-Li , I have added the cookies part here (full-validation from browser cookies)

rc4_inappchat1.mp4

But we are facing a major roadblock, i.e, with the previous approach (with CSR approach) after a certain interval the requests are being revalidated which as I have discussed with Rohan sir and he agreed that that might not be the best approach. So it is still to be figured out. :(

@Sing-Li
Copy link
Member

Sing-Li commented Feb 20, 2022

Cool. @sidmohanty11 Let's go with the current way of handling (hard coded ENVIRONMENT VAR) for now. Is it OK for me to merge this down now?

@sidmohanty11
Copy link
Contributor Author

@Sing-Li I can work on that on a separate PR, and as UI needs a lot of work I think @demonicirfan can take it from here

@Sing-Li Sing-Li merged commit a661f44 into RocketChat:master Feb 21, 2022
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.

None yet

2 participants