Captions reengineering. #910
Conversation
diegoreymendez
added
[Type] Enhancement
and removed
[Status] Don't Merge Yet
[Status] Not Ready For Review
labels
jleandroperez
left a comment
jleandroperez
left a comment
There was a problem hiding this comment.
Love these changes @diegoreymendez !!!. Found a minor, tiny glitch:
- Launch Empty Editor
- Insert Image
- Open Image's Properties
- Type a
Captionsnippet - Switch to HTML. Verify there's a figcaption tag
- Back to WYSIWYG mode. Open the Image's Properties
Result:
The Caption field is empty
| @@ -0,0 +1,39 @@ | |||
| import UIKit | |||
|
|
|||
| extension Dictionary where Key == NSAttributedStringKey, Value == Any { | |||
|
|
||
| /// Internal convenience helper. Returns the internal string as a NSString instance | ||
| /// | ||
| var foundationString: NSString { |
There was a problem hiding this comment.
I was sure this was already somewhere else
There was a problem hiding this comment.
@jleandroperez if you look a bit lower down in the PR you will see that Diego is just moving this around.
| } | ||
|
|
||
|
|
||
|
|
| } | ||
| } | ||
|
|
||
|
|
|
Thanks for the feedback! |
| let linkInfo = richTextView.linkInfo(for: attachment) | ||
| let linkRange = linkInfo?.range | ||
| let linkURL = linkInfo?.url | ||
| let linkUpdateRange = linkRange ?? richTextView.textStorage.ranges(forAttachment: attachment).first! |
There was a problem hiding this comment.
Are we sure we want to use the ! on this line?
|
Look like the UI Tests are failing. Not sure if this PR is to blame for that. |
|
The UI tests have been failing on and off, in a not very consistent manner. It seems like they're very sensitive to not only change, but also how the simulator is set up. I was planning (if we agree) to tackle that separately because I've been seeing that behavior event before this PR. |
|
While in general this PR is working very well I want to raise the following issues:
|
SergioEstevao
left a comment
SergioEstevao
left a comment
There was a problem hiding this comment.
Just want to be sure if the issues that I found are not blockers for merging this functionality.
|
Another issue that I found is the presentation of caption, it looks we are adding a lot of paragraph spacing between the image and the caption. This make it look that the caption is not attached to the image. |
|
@jleandroperez - Can you try again? I found an issue with some paragraph-separators that were being lost on conversion. |
|
@SergioEstevao - I'll try to reduce the spacing and add a specific style to captions. Please hold tight. |
jleandroperez
left a comment
jleandroperez
left a comment
There was a problem hiding this comment.
Amazing work Diego, behaves as advertised!!!
|
@SergioEstevao - This PR now styles captions differently. Also when pressing enter in the middle of a caption, the style will be removed for the right-hand text. @SergioEstevao, @jleandroperez - Can I ask you for another run? |
|
PS: right now the caption styling is defined by |
|
@diegoreymendez from my tests, it looks that now if you add a link to an image |
|
Very nice find, changes are up and ready for review. |
SergioEstevao
left a comment
SergioEstevao
left a comment
There was a problem hiding this comment.
Excellent work @diegoreymendez. I will sort our the link bug on a separate issue and PR.
|
Thanks @SergioEstevao ! |
Description:
This PR re-engineers captions in order to support a wider range of caption options (they could potentially be used for more than images now).
The old architecture was also causing some trouble when combining links with captions, which this PR will allow us to solve more elegantly.
To see the problem, simply go to
developand try setting a link for a captioned image. The link is applied to the caption text too and there's no easy fix for it.Additional details:
Captions are now editable inline too, which is a good thing.
Captions now support links!
Demo:
Updated:
Testing:
Just try to break captions in whatever way you deem possible. Don't hesitate to take your time to test this PR, as it includes important architectural changes to the way captions work.