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

Not displaying unsupported emoji in the emoji picker and mention plugin. #17698

Open
wants to merge 4 commits into
base: epic/ck/17361
Choose a base branch
from

Conversation

martnpaneq
Copy link
Contributor

Suggested merge commit message (convention)

Internal: Not displaying unsupported emoji in the emoji picker and mention plugin. Closes #17666.


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

@martnpaneq martnpaneq force-pushed the ck/17666-hide-unsupported-emoji branch from 5f03ccc to de14e29 Compare December 30, 2024 12:41
@martnpaneq
Copy link
Contributor Author

This solution bases on the solution used in emoji-picker-element.

@martnpaneq
Copy link
Contributor Author

Testing this would be hard, because we would have to mock an unsupported emoji. I'm not adding any tests for now - if anyone thinks I should add them, let me know.

@martnpaneq martnpaneq force-pushed the ck/17666-hide-unsupported-emoji branch 2 times, most recently from 4e308f6 to 9149e61 Compare December 30, 2024 15:19
@martnpaneq martnpaneq force-pushed the ck/17666-hide-unsupported-emoji branch from 9149e61 to e817e87 Compare December 30, 2024 15:30
Copy link
Contributor

@psmyrek psmyrek left a comment

Choose a reason for hiding this comment

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

It looks like most of the emojis with flags have been filtered out on Windows, even though they were displayed correctly.

Before:

before

After:

after

The strange [C][Q] emoji should be filtered but it is still available.

Copy link
Contributor

@psmyrek psmyrek left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

I didn't find any unsupported emoji. All other emojis are displayed correctly.

obraz

obraz

Copy link
Member

@pomek pomek left a comment

Choose a reason for hiding this comment

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

R- from my side, due to missing options (approaches to meet the requirements and goals), cons, pros, and conclusions (what, why, etc.) under the issue.

* Creates a div for emoji width testing purposes.
*/
private _createEmojiWidthTestingContainer(): HTMLDivElement {
const container = document.createElement( 'div' );
Copy link
Member

Choose a reason for hiding this comment

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

To avoid processing, I would mark this element with a proper aria attribute.

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.

3 participants