Skip to content

Conversation

@SergioEstevao
Copy link
Contributor

Only keep references that are done on the demo project.

There is only things that I see as issues if we remove Gridicons from the pod:

  • We don't get a default Missing Image on the TextView -> but this could be set for however is using the pod
  • We don't get a play icon on the the Video attachment, I was considering implementing a default play button icon using some CoreGraphics code ( a triangle with round corners)

If we agree with this solution I can go ahead and implement the play icon and remove the Gridicon dependency.

To test:

  • Build the pod
  • Check if all tests build and pass
  • Build the example
  • Run the example and check if nothing breaks.

Only keep references that are done on the demo project.
@diegoreymendez
Copy link
Contributor

While having no dependencies is very convenient, I don't want to do it in detriment of not having default images.

The only scenario in which I'd consider removing those defaults is if we could force the client app to set those defaults.

Unfortunately, I don't think it's very feasible since our TextView could be instantiated in a number of different ways (in some cases with the initializer not taking any input parameters from the client app).

@SergioEstevao
Copy link
Contributor Author

To be honest at the moment if you don't set an attachment delegate the app will assert crash when trying to render an image. And that delegate is also not initiated by default.

The default image could be one of the pre-conditions also.

@diegoreymendez
Copy link
Contributor

@SergioEstevao - I'm ok with having the client app provide the default images through a delegate, but if that's the idea I'd need us to do the full conversion.

That is:

  • You'd have to add the delegates and assertions in this PR,
  • Once it's merged you'd have to integrate it into WPiOS to make sure we provide those defaults.

If that sounds fine then let me know when the changes are up.

@SergioEstevao
Copy link
Contributor Author

@diegoreymendez and/or @jleandroperez ready for another look.

Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

There are several spots in which the default image was removed, but the new ones are not being set.

private extension BlockquoteFormatterTests {
var testTextView: TextView {
let view = TextView(defaultFont: UIFont.systemFont(ofSize: 14), defaultMissingImage: Gridicon.iconOfType(.image))
let view = TextView(defaultFont: UIFont.systemFont(ofSize: 14), defaultMissingImage: UIImage())
Copy link
Contributor

Choose a reason for hiding this comment

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

We're removing the default here, but not assigning a new 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.

Test file

switch attachment {
case _ as ImageAttachment:
placeholderImage = Gridicon.iconOfType(.image, withSize: imageSize)
placeholderImage = UIImage()
Copy link
Contributor

Choose a reason for hiding this comment

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

We're removing the default here, but not assigning a new 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.

Test file.

placeholderImage = UIImage()
case _ as VideoAttachment:
placeholderImage = Gridicon.iconOfType(.video, withSize: imageSize)
placeholderImage = UIImage()
Copy link
Contributor

Choose a reason for hiding this comment

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

We're removing the default here, but not assigning a new one.

placeholderImage = UIImage()
default:
placeholderImage = Gridicon.iconOfType(.attachment, withSize: imageSize)
placeholderImage = UIImage()
Copy link
Contributor

Choose a reason for hiding this comment

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

We're removing the default here, but not assigning a new 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.

Test File.


func createEmptyTextView() -> Aztec.TextView {
let richTextView = Aztec.TextView(defaultFont: UIFont.systemFont(ofSize: 14), defaultMissingImage: Gridicon.iconOfType(.attachment))
let richTextView = Aztec.TextView(defaultFont: UIFont.systemFont(ofSize: 14), defaultMissingImage: UIImage())
Copy link
Contributor

Choose a reason for hiding this comment

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

We're removing the default here, but not assigning a new 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.

Test file.


func createTextView(withHTML html: String) -> Aztec.TextView {
let richTextView = Aztec.TextView(defaultFont: UIFont.systemFont(ofSize: 14), defaultMissingImage: Gridicon.iconOfType(.attachment))
let richTextView = Aztec.TextView(defaultFont: UIFont.systemFont(ofSize: 14), defaultMissingImage: UIImage())
Copy link
Contributor

Choose a reason for hiding this comment

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

We're removing the default here, but not assigning a new 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.

Test file.

func createTextViewWithContent() -> Aztec.TextView {
let paragraph = "Lorem ipsum dolar sit amet.\n"
let richTextView = Aztec.TextView(defaultFont: UIFont.systemFont(ofSize: 14), defaultMissingImage: Gridicon.iconOfType(.attachment))
let richTextView = Aztec.TextView(defaultFont: UIFont.systemFont(ofSize: 14), defaultMissingImage: UIImage())
Copy link
Contributor

Choose a reason for hiding this comment

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

We're removing the default here, but not assigning a new 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.

Test File

func testTextViewReferencesStorage() {

let textView = Aztec.TextView(defaultFont: UIFont.systemFont(ofSize: 14), defaultMissingImage: Gridicon.iconOfType(.attachment))
let textView = Aztec.TextView(defaultFont: UIFont.systemFont(ofSize: 14), defaultMissingImage: UIImage())
Copy link
Contributor

Choose a reason for hiding this comment

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

We're removing the default here, but not assigning a new 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.

Test file.


func testMaxIndex() {
let textView = Aztec.TextView(defaultFont: UIFont.systemFont(ofSize: 14), defaultMissingImage: Gridicon.iconOfType(.attachment))
let textView = Aztec.TextView(defaultFont: UIFont.systemFont(ofSize: 14), defaultMissingImage: UIImage())
Copy link
Contributor

Choose a reason for hiding this comment

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

We're removing the default here, but not assigning a new 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.

Test file.


func testAdjustedIndex() {
let textView = Aztec.TextView(defaultFont: UIFont.systemFont(ofSize: 14), defaultMissingImage: Gridicon.iconOfType(.attachment))
let textView = Aztec.TextView(defaultFont: UIFont.systemFont(ofSize: 14), defaultMissingImage: UIImage())
Copy link
Contributor

Choose a reason for hiding this comment

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

We're removing the default here, but not assigning a new 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.

Test file.

Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

Good job!

@SergioEstevao SergioEstevao merged commit c26bfa8 into develop Jun 27, 2017
@SergioEstevao SergioEstevao deleted the gridicon_cleanup branch June 27, 2017 21:20
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.

4 participants