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

feat: #6925 - Emoji selection when typing : (colon) symbol in the chat box #6991

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

ElSalvo96
Copy link

@ElSalvo96 ElSalvo96 commented Oct 18, 2024

About this PR

This PR adds a new feature for selecting emoji when typing a colon in the chat box; for example, typing :joy will show a selection of emoji to choose from.

This PR should close this issue: #6925

The list of emoji is generated from /platform/plugins/text-editor-resources/src/components/extension/emojiIdMapGenerator.ts, a revisited file from https://github.com/ikatyang/emoji-cheat-sheet/blob/master/scripts/fetch.ts

Before

image

After

image
image

Note: Since this is my first experience using Svelte, I would appreciate any input.

Signed-off-by: ElSalvo96 <[email protected]>
Copy link

@kasir-barati kasir-barati left a comment

Choose a reason for hiding this comment

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

Hi,

Thanks for the PR. I jot down some pointer I could put my finger on. But since I do not know svelte I could not help you with that.

plugins/text-editor-resources/src/Completion.ts Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

[Non-blocking comment]: In this file I have seen 3 eslint-disable-next-line. I do not wanna say do not use it, but rather I prefer to stay safe as much as possible. So for example if the type is an issue I try first to fix that and not use eslint-disable-next-line.

plugins/text-editor-resources/src/components/Popup.svelte Outdated Show resolved Hide resolved
insertTable (options: { rows?: number, cols?: number, withHeaderRow?: boolean }) {
insertTable(options: { rows?: number; cols?: number; withHeaderRow?: boolean }) {

Choose a reason for hiding this comment

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

To me it looks like someones IDE is not configured to format the code properly. Do we need some kind of pipeline to reject unformatted code? Or maybe husky to format the code before push?

Copy link
Author

@ElSalvo96 ElSalvo96 Oct 19, 2024

Choose a reason for hiding this comment

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

There is no husky in this project.
The file is saved with the prettier-eslint extension. So I think that my new one is right.

Choose a reason for hiding this comment

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

Yeah, I meant that it should be added otherwise these kind of unnecessary changes will be pushed and someone has to code review these kind of changes while we can prevent it simply by adding something like husky. I do not know how ususally thing are done here but if it were up to me I would have created a task called add husky or at least do some R&D around it to find out whether it worth the effort or not.

Choose a reason for hiding this comment

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

Could not we just remove this file?

You know what I mean, it was added in this PR and if it is not needed then should not be here as well.

Copy link
Author

Choose a reason for hiding this comment

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

Yes I could. I was put there because no one know if in the future is still need to generate the file. With that instead of going into the thought process, you only need to run this script.

In any case, do you believe that deleting it is necessary? It's different approach it depends on how do they usually work.
Let me know

Choose a reason for hiding this comment

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

Hmm, cannot we just doc it instead if it is really something useful somewhere else? Dunno what kind of documentation generating tool we use here but it could go there if you ask me.

@ElSalvo96
Copy link
Author

Hello @kasir-barati, thanks for the review.

When I did this feature, I just copied from other components, avoiding editing too much of the logic itself. 
In that moment, I didn't care about the type inference or some other stuff like the eslint rules. I was more focused on shipping the feature faster without logical error. General speaking, I think that there is a lot of redundant code that can be optimized, but knowledge about the framework is still limited. 

In any case, I feel that you were right into a lot of the comment.

ElSalvo96 and others added 4 commits October 19, 2024 15:44
Co-authored-by: Mohammad Jawad (Kasir) Barati <[email protected]>
Signed-off-by: ElSalvo96 <[email protected]>
Co-authored-by: Mohammad Jawad (Kasir) Barati <[email protected]>
Signed-off-by: ElSalvo96 <[email protected]>
Co-authored-by: Mohammad Jawad (Kasir) Barati <[email protected]>
Signed-off-by: ElSalvo96 <[email protected]>
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