Skip to content

Notification Rich Text Support#2735

Merged
jleandroperez merged 66 commits intodevelopfrom
issue/2661-notification-comments-rich-text-support
Nov 11, 2014
Merged

Notification Rich Text Support#2735
jleandroperez merged 66 commits intodevelopfrom
issue/2661-notification-comments-rich-text-support

Conversation

@jleandroperez
Copy link
Contributor

Notifications: Rich Text Support

Now Text and Comment notifications have support for inline images. We've implemented a simple component, RichTextView, based on TextKit.

The original goal was to wire RichTextView's delegate to asynchronously download the images. However, that breaks if the image size is originally unknown: the user may / may not specify the embedded image size, beforehand.

For that reason, a simple helper class, NotificationMediaDownloader was implemented: this class receives a list of URL's, and will hit a completion callback whenever the assets are downloaded.

Duplicate downloads are prevented, and a simple cache has been implemented.

Closes #2661

/cc @aerych + @dan may i bother you with a code review?.
Thanks in advance!

…ation-comments-rich-text-support

Conflicts:
	WordPress/Classes/ViewRelated/Notifications/Notifications.storyboard
@jleandroperez
Copy link
Contributor Author

@aerych Thanks for such an epic testing session!!. All of the issues pointed out should be good now (details below).

Scenario 1

HTML in comments is expected to be filtered by wpcom, unless you're an admin in the blog.
Regarding the vertical spacing, couldn't reproduce it. But i suspect that was due to an autolayout bug that got fixed in one of the latest commits.

Scenario 2

Links to images will not be rendered as images (that'd be expected). Users would be required to use the img html tag.

Scenario 3

Fixed! It was the NSParagraphStyle's maxLineHeight breaking the attachment.

Scenario 4

Autolayout error: fixed, thanks!

Scenario 5

Same as scenario 3, caused by NSParagraphStyle's maxLineHeight.

Plus:

  • Image size is now capped dynamically to fit onscreen
  • Will add a ticket for the WPImageViewController feature, thanks!

Thank you sir!

Copy link
Contributor

Choose a reason for hiding this comment

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

:D

@aerych
Copy link
Contributor

aerych commented Nov 11, 2014

Testing this round is looking good. I noticed just one small bit of wonkiness. To reproduce:

  • Start out in portrait orientation
  • Find a comment with lots of text and one or more images.
  • Scroll to the bottom of the comment.
  • Rotate to landscape
  • Rotate back to portrait orientation

You'll probably see a big gap of white space where before there was none. Not a huge deal but if its an easy fix we should do it. Otherwise I'd say this is good to go, so, squash that one (if possible) then :shipit: when ready. :)

@jleandroperez
Copy link
Contributor Author

@aerych (Sorry about bothering you, yet again). Whenever possible, would you please verify the fix?.

Thank you!

  • Implemented a Max Retry count, just as a safety measure, so the downloader never ends up in a loop.

@aerych
Copy link
Contributor

aerych commented Nov 11, 2014

Looks good :D :shipit:

@jleandroperez
Copy link
Contributor Author

@aerych thank you sir!

jleandroperez added a commit that referenced this pull request Nov 11, 2014
…n-comments-rich-text-support

Notification Rich Text Support
@jleandroperez jleandroperez merged commit ded11ea into develop Nov 11, 2014
@jleandroperez jleandroperez deleted the issue/2661-notification-comments-rich-text-support branch November 11, 2014 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Notifications Rich Text Support

2 participants