Skip to content

Conversation

@danielebogo
Copy link
Contributor

Refs. WPiOS #12462

This PR exposes the Editor html storage as let property in order to customize its colors due to the introduction of DarkMode. This is required for implementing dark mode in WPiOS.

To test:

• Run the Unit Tests
• Run WPiOS #12462

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Looks great to me!

Tested via wordpress-mobile/WordPress-iOS#12462 and it works perfectly 🎉

I added a comment but nothing really blocking. 👍

Pod::Spec.new do |s|
s.name = 'WordPress-Aztec-iOS'
s.version = '1.8.1'
s.version = '1.9.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should be using 1.9.0-beta.1

XCTAssertEqual(closingTagColor, HTMLStorage.Styles.defaultTagColor)
}

func testSetTextColor() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the tests! 🙏

public class EditorView: UIView {
public let htmlTextView: UITextView
public let richTextView: TextView
public let htmlStorage: HTMLStorage
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it would be better to have a computed property instead? Something like

public var htmlStorage: HTMLStorage {
    guard let htmlStorage = htmlTextView.textStorage as? HTMLStorage else {
        fatalError("If this happens, something is very off on the init config")
    }
    return htmlStorage
}

I don't particularly like the fatalError, even though the guard should always succeed. Alternatively we can make it an optional, but that wouldn't make much sense for the "user", and how to handle the hypothetical nil case?

But it would remove the hassle with the coder init.

None look like a great option to me really, so I'm happy to keep it as it is if you think it is the best one 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep I like it!

@danielebogo danielebogo requested a review from etoledom September 4, 2019 16:44
@danielebogo
Copy link
Contributor Author

danielebogo commented Sep 4, 2019

Thanks @etoledom for reviewing this! I updated the property and the Podspecs. Tests passed successfully!

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Looks great!
Thanks for the updates 🙏

@danielebogo danielebogo merged commit 9224f58 into develop Sep 4, 2019
@SergioEstevao SergioEstevao deleted the feature/expose-editor-html-storage branch April 20, 2020 15:06
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.

3 participants