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

Default to no emoji when rendering Markdown #850

Conversation

savetheclocktower
Copy link
Contributor

Identify the Bug

I saw this linter message when I opened the package-backend project and grew curious:

Screenshot 2023-12-30 at 4 49 47 PM

Turns out that emoji is an interpretation of :@.

I am guessing that most people who render Markdown would not expect that the character sequence :@ would, in all cases, be converted to an emoji. So let's make that an opt-in feature.

Description of the Change

Change the default for useDefaultEmoji in atom.ui.markdown.render to false.

Alternate Designs

I thought about removing this option altogether out of spite, but I calmed down.

Possible Drawbacks

Nah.

Verification Process

In the devtools console, run

atom.notifications.addError("foo", { description: ":@" });

and inspect the output. On master it'll be a smiley; on this PR branch it'll be the literal text :@.

Release Notes

Switch default to false for converting ASCII emoticons to emoji when rendering Markdown.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

This looks fine, as it's a simple change.

But since markdown-preview expects to be able to use emojis, would you mind added the option there to enable emojis?

Should be ./packages/markdown-preview/lib/renderer.js, lines 46, and 78

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Looks awesome to me!
Lets just wait for some tests to run, then I think we are good to merge

@savetheclocktower savetheclocktower merged commit 69e4224 into pulsar-edit:master Jan 3, 2024
99 checks passed
@Daeraxa Daeraxa mentioned this pull request Jan 16, 2024
12 tasks
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