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.

Sign up for GitHub

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

Jump to bottom

Rendering emojis in MarkdownTextBlock #1625

Merged
merged 6 commits into from
Nov 15, 2017
Merged

Rendering emojis in MarkdownTextBlock #1625

merged 6 commits into from
Nov 15, 2017

Conversation

ivan-stp
Copy link
Contributor

@ivan-stp ivan-stp commented Nov 15, 2017 •
edited
Loading

Issue: #1515

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build or CI related changes
[ ] Documentation content changes
[ ] Sample app changes
[ ] Other... Please describe:

What is the current behavior?

No emoji support in MarkdownTextBlock

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Docs have been added / updated (for bug fixes / features)
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)

What is the new behavior?

Emojis are rendered in MarkdownTextBlock
image

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

michael-hawker
michael-hawker reviewed Nov 15, 2017
View reviewed changes
Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/Display/XamlRenderer.cs Outdated
{
var emoji = new Run
{
FontFamily = EmojiFontFamily ?? new FontFamily("Segoe UI Emoji"),
Copy link
Member

Choose a reason for hiding this comment

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

Make the end of this a private const like 'default emoji font'.

michael-hawker
michael-hawker previously requested changes Nov 15, 2017
View reviewed changes
Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Should update sample for markdown text box to include an example.

ivan-stp
ivan-stp commented Nov 15, 2017
View reviewed changes
Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/Parse/Inlines/EmojiInline.cs Outdated

// Codes taken from https://gist.github.com/rxaviers/7360908
// Ignoring not implented symbols in Segoe UI Emoji font (e.g. :bowtie:)
private static readonly Dictionary<string, int> _emojiCodesDictionary = new Dictionary<string, int>
Copy link
Contributor Author

@ivan-stp ivan-stp Nov 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@michael-hawker Is it ok to have this dictionary here or should I make this class partial and move dictionary to another file?

Copy link
Member

Choose a reason for hiding this comment

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

I was actually wondering how we should manage it. Splitting it out at least into a separate partial class is a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need an _emojiCodesDictionary ?

Copy link
Member

Choose a reason for hiding this comment

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

@hermitdave it's what translates :smile: into the actual character needed by the font for display 😄.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh crap.. GitHub didn't show me that file at all.

@michael-hawker michael-hawker dismissed their stale review November 15, 2017 17:00

I'm at an event today, so dismissing to not block future integration. Someone else should sign-off when changes are made. Thanks!

nmetulev
nmetulev requested changes Nov 15, 2017
View reviewed changes
Copy link
Contributor

@nmetulev nmetulev left a comment

Choose a reason for hiding this comment

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

This is amazing. Other than few formatting warnings, looks great. If you can get these updated today, we can get this in 2.1 :)

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/Helpers/Common.cs Outdated
@@ -109,7 +114,7 @@ static Common()
SuperscriptTextInline.AddTripChars(_triggerList);
CodeInline.AddTripChars(_triggerList);
ImageInline.AddTripChars(_triggerList);

EmojiInline.AddTripChars(_triggerList);
// Create an array of characters to search against using IndexOfAny.
Copy link
Contributor

Choose a reason for hiding this comment

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

Single line comment must be preceded by blank line

Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/Parse/Inlines/EmojiInline.cs Outdated

namespace Microsoft.Toolkit.Uwp.UI.Controls.Markdown.Parse
{
internal partial class EmojiInline : MarkdownInline, IInlineLeaf
Copy link
Contributor

Choose a reason for hiding this comment

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

partial elements must be documented

nmetulev
nmetulev reviewed Nov 15, 2017
View reviewed changes
Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/Parse/MarkdownInline.cs Outdated
Image
Image,

///<summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation line must begin with space

nmetulev
nmetulev reviewed Nov 15, 2017
View reviewed changes
Microsoft.Toolkit.Uwp.UI.Controls/MarkdownTextBlock/Parse/Inlines/EmojiInline.EmojiCodes.cs Outdated

namespace Microsoft.Toolkit.Uwp.UI.Controls.Markdown.Parse
{
internal partial class EmojiInline
Copy link
Contributor

Choose a reason for hiding this comment

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

Partial elements must be documented

@ivan-stp
Copy link
Contributor Author

@nmetulev Fixed formatting issues

@nmetulev nmetulev merged commit 1c25fe6 into CommunityToolkit:master Nov 15, 2017
@ivan-stp ivan-stp deleted the markdown_emoji branch November 15, 2017 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-hawker michael-hawker michael-hawker left review comments

@hermitdave hermitdave hermitdave approved these changes

@nmetulev nmetulev nmetulev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ivan-stp @nmetulev @hermitdave @michael-hawker