Skip to content

Conversation

@chinesedfan
Copy link
Contributor

Fixes #60.

In fact, emoji was allowed after the entry description by #29.

@transitive-bullshit
Copy link
Collaborator

Overall, looks really solid. See my comment in #60 as Sindre has the final call on all formatting decisions like this.

@sindresorhus
Copy link
Owner

#60 (comment)

const emoji = match[0];
const codePoints = [...emoji].length;
if (match.index === 0) {
text = text.substr(codePoints).trim();
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer using slice instead of substr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. But sadly you rejected this feature, #60 (comment).

}
}

return text;
Copy link
Owner

Choose a reason for hiding this comment

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

Why not just text.replace(emojiRegex, '')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my test cases. I want to trim emojis and spaces, but NOT to remove inner emojis and NOT to trim spaces if no emojis.

  • t.deepEqual(trimEmoji('⭐ hello'), 'hello');
  • t.deepEqual(trimEmoji(' hello '), ' hello ');
  • t.deepEqual(trimEmoji('hello⭐hello'), 'hello⭐hello');

@transitive-bullshit
Copy link
Collaborator

transitive-bullshit commented Apr 7, 2019

@chinesedfan can you update this PR to reflect the decision made in #60?

Thanks!

@chinesedfan
Copy link
Contributor Author

@transitive-bullshit Updated.

@transitive-bullshit transitive-bullshit changed the title Allow emoji in section header and image after the entry description Allow emoji or image after the entry description Apr 8, 2019
@sindresorhus sindresorhus merged commit 229230a into sindresorhus:master Apr 19, 2019
@sindresorhus
Copy link
Owner

Thanks :)

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.

Allow emoji or image after the entry description

3 participants