Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Swift Package Manager supported equally with Cocoapods, dedicated SPM & cocoapods example projects, lots of maintenance & cleanup. #7

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

DanBurkhardt
Copy link

Thanks for maintaining & sharing your version, I think this PR will improve usability for a lot of other devs by adding full support for SPM while maintaining support for Cocopods.

It looks like a big one, but it’s really not too complicated, most of the files and changes you see are just moving things around and adding one additional example project. I’m happy to take any suggestions, ofc.

Let me know what you think!

DanBurkhardt and others added 7 commits February 2, 2023 22:34
… gain a contextually informed reference to the actual bundle of a framework (SPM or Cocoapods), de-flattened assets directory as unnecessary, implemented helper for retrieving web and image assets within the RichEditorView, fixed a bug that caused erratic scrolling when the keyboard appears while editing the view, some comment documentation, podspec minor version bump due to new features with no breaking changes, updated gitignore for SPM ignores, import statement fix
Swift Package Manager Implementation
editorView.placeholder = "Edit here"
// disable scroll to fix issue with auto-scrolling when keyboard appears
editorView.isScrollEnabled = false
let html = "<b>Jesus Saves!.</b> No words of praise, no promised land to take you to-- there is no other WAY! <a href='https://slayer.net'>perfectGod.com</a>"
Copy link
Owner

Choose a reason for hiding this comment

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

It'd be better to totally remove this html instead of "masking" it, if you didn't like what it originally said :)

Copy link
Author

Choose a reason for hiding this comment

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

I totally forgot that was in there! I guess it was a good test for how deep the review would go 😆

I'll update that in a moment, I suppose we can't all be Slayer fans so removal makes sense 👍

Copy link
Owner

Choose a reason for hiding this comment

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

Fair enough.

Also, in light of:

no promised land to take you to

I'd love to know what you mean by that statement, if you're willing to discuss or defend it DM me @besswisdom. I see you prefer Threads, but I'm not on there.

Copy link
Author

Choose a reason for hiding this comment

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

@cbess I pushed an quick update swapping to the less controversial Quick Brown Fox boiler plate. Hoping that is amenable for the pr review, and thanks again.

Copy link
Author

@DanBurkhardt DanBurkhardt Oct 29, 2023

Choose a reason for hiding this comment

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

I'd love to know what you mean by that statement, if you're willing to discuss or defend it DM me @besswisdom. I see you prefer Threads, but I'm not on there.

Oh, they are the lyrics to a song by Slayer (great band but a very specific type of music). I think I was testing something while running the Example project and changed the text to make sure I was looking at the right version of the view controller, but tried to keep it religious. That lyric floated across my mind so I used it, and then I forgot about it, sorry for the mixup!

I'm also happy to revert it back, just lmk. I figured changing to something neutral would be better than deleting it.

@cbess
Copy link
Owner

cbess commented Oct 31, 2023

@DanBurkhardt Thanks for your contributions! I'd be fine, and even prefer, to omit Cocoapods and have it only as a SPM package. Thoughts?

Overall the changes are good with me, minus the gospel text omissions.

@DanBurkhardt
Copy link
Author

DanBurkhardt commented Oct 31, 2023

Thanks @cbess! I think it's still too early because a lot of people are just switching to SPM now. Where both can be supported, I think it is still preferable.

@DanBurkhardt
Copy link
Author

@cbess checking in on this-- lmk if you need anything else, but I would keep cocoapods compatibility in place for the time being.

@cbess
Copy link
Owner

cbess commented Nov 5, 2023

@DanBurkhardt will do. I'm going to add a couple updates to your PR, then I'll merge it. Hopefully sometime next week


# ignore sample project pod stuff
Examples/Cocoapods/Pods/
Examples/Cocoapods/Podfile.lock
Copy link
Owner

Choose a reason for hiding this comment

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

Preferred, would be to omit the Examples/Cocoapods portion and simplify it to use:
Pods/
Podfile.lock

@@ -80,7 +80,7 @@ Most basic use:

```
editor = RichEditorView(frame: view.bounds)
editor.html = "<h1>Jesus is God.</h1> He died for our sins and rose from the dead by His own power. Repent and believe the gospel!"
Copy link
Owner

Choose a reason for hiding this comment

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

Please restore this readme text. I didn't mind that you changed the demo text in the controller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants