Skip to content
This repository has been archived by the owner on Apr 18, 2023. It is now read-only.

Feat new html rendering #224

Merged
merged 16 commits into from
Mar 1, 2022
Merged

Feat new html rendering #224

merged 16 commits into from
Mar 1, 2022

Conversation

maicon-brauwers
Copy link
Collaborator

@maicon-brauwers maicon-brauwers commented Feb 25, 2022

Description

There are still things that we'll be done in follow up tickets:

  • Add missing styling (lists, blockquote, align and some more stuff)
  • Add tests for the new renderer
  • Fix bug of embedding italics inside bold (or vice-versa)

DevQA

DevQA Prep

  • Run pod install
  • Test RichTextView to ensure that it passes (unit and UI tests)
  • Navigate to the Example project in the root Example folder and run the app
  • Ensure that everything in the Example app looks visually correct

DevQA Steps

Comments

@@ -12,6 +12,7 @@ let package = Package(
.package(name: "Down", url: "https://github.com/johnxnguyen/Down", .upToNextMajor(from: "0.11.0")),
.package(name: "SnapKit", url: "https://github.com/SnapKit/SnapKit", .upToNextMajor(from: "5.0.1")),
.package(name: "iosMath", url: "https://github.com/tophatmonocle/iosMath", .upToNextMajor(from: "1.1.1")),
.package(name: "SwiftRichString", url: "https://github.com/tophatmonocle/SwiftRichString", .branch("master"))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pointing to our fork given that we'll need to fix some bugs on the RichString lib

import UIKit
import SwiftRichString

class DynamicAttributesResolver: XMLDynamicAttributesResolver {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This class is a delegate that handles unknown tags and custom attributes.

* Renders an HTML string into a NSAttributedString by using the SwiftRichString dependency
*/

class HTMLRenderer {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Core class responsible for the rendering; it generates the styles if needed and then call the SwiftRichString set:style method


// This class is responsible to create the styles that are using to style the HTML tags

struct HTMLStyleBuilder {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This builds the styles. Not all required styles are here - will work on it on a follow up ticket.

@@ -41,6 +41,8 @@ public class RichTextView: UIView {
textViewDelegate: RichTextViewDelegate? = nil,
customAdditionalAttributes: [String: [NSAttributedString.Key: Any]]? = nil,
frame: CGRect,
shouldUseOptimizedHTMLParsing: Bool = false,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Parameters to control the version of rendering we should be using

@@ -127,7 +135,7 @@ class RichTextParser {
if attributes.isEmpty || attributes[.attachment] != nil {
return
}
let specialDataSubstring = specialDataTypesString.string[
let specialDataSubstring: String = specialDataTypesString.string[
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To disambiguate

@@ -220,6 +231,27 @@ class RichTextParser {
return (finalOutputString, nil)
}

private func getRichTextWithHTMLAndMarkdownHandledV2(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

V2 shorter method

@maicon-brauwers maicon-brauwers added the reviewme Indicates a PR that needs to be reviewed label Feb 25, 2022
Copy link
Contributor

@Noobish1 Noobish1 left a comment

Choose a reason for hiding this comment

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

Just a few comments/questions

Source/HTML Rendering/HTMLRenderer.swift Show resolved Hide resolved
finalStyleToApply.color = Color(hexString: value)
case "style":
// THIS IS ALL HORRIBLE AND SHOULD BE REFACTOR USING A STRING SCANNER OR REGEX!!!
if value.starts(with: "color: rgb(") {
Copy link
Contributor

Choose a reason for hiding this comment

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

will this be changed later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I will change it later on follow up tickets.

}

let codeStyle = baseStyle.byAdding {
$0.backColor = Color(red: 249/255, green: 249/255, blue: 249/255, alpha: 1.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

these should be using tokens probably? which means they'd need to be passed in

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is true. The PR really captures work in progress, and we will complete it on follow up tickets.

But you are right - we probably should add color parameters to the HTMLStyleParams struct.
Or move the responsibility of build styles to the application instead of the framework.

Copy link
Contributor

@Noobish1 Noobish1 left a comment

Choose a reason for hiding this comment

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

LGTM

@maicon-brauwers maicon-brauwers merged commit c34d4d8 into master Mar 1, 2022
@maicon-brauwers maicon-brauwers deleted the feat-new-html-rendering branch March 1, 2022 14:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
reviewme Indicates a PR that needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants