-
Notifications
You must be signed in to change notification settings - Fork 326
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
Help chat #7151
Help chat #7151
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super cool! A few questions:
- Where is server implementation? We should keep it in our repo as well.
- The design is not exactly as in Figma, do we plan to fix it in this PR or in a separate one?
- Where on Discord the user messages land? Do we have access to it, or it is in a test server for now?
// ============= | ||
|
||
/** All possible reaction emojis. */ | ||
type ReactionSymbol = '❤️' | '🎉' | '👀' | '👍' | '👎' | '😀' | '🙁' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a repeated code - above we have the same. Can we refactor it, please?
// === Message Types === | ||
// ===================== | ||
|
||
// FIXME[sb]: Consider deduplicating. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be part of cleaning this PR if its about code deduplication :) if its about anything else, it needs better docs.
} | ||
|
||
/** Chat sidebar. */ | ||
function Chat(props: ChatProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function is 400 lines long! Can we refactor it to several well named functions instead, please?
|
400 line long function: I'll see whst i can do |
|
re: using a npm library - it would probably work, but upon some investigation it seems like it's possible to use git dependencies, which is probably more convenient, since it wouldn't require publishing new tags for local development also, it's not like it should be publicly visible on npm - not that it can't be, but i do think what determines whether something belongs on npm should be an intentional decision, rather than "we have to put it on npm for things to work" - when there are other options, at least |
Oh, I did not know that nom supports git dependencies. This is way better solution, let's use it! :) |
|
still having issues that threads are not appending to the list but only current is visible. now even reopening doesn't help Screen.Recording.2023-07-07.at.16.00.15.mov |
|
|
i just realized, i don't think this works with multiple users in an organization. i'll have to take a look at that sometime |
this one is not obvious. lets fix conflicts and open for review and merge (deploy) this implementation then we see |
Pull Request Description
Closes #6610
Important Notes
Screencasts
screen-recorder-mon-jul-03-2023-20-29-41.webm
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
The documentation has been updated, if necessary.Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.If GUI codebase was changed, the GUI was tested when built using./run ide build
.