soffes / MarkdownKit

Fancy Markdown input with TextKit powered by CommonMark
MIT License
45 stars 6 forks source link

Compatibility improvements (iOS version and module name) #5

Closed josjevv closed 4 years ago

josjevv commented 4 years ago

Hi,

not sure if you are open to these changes, your library seems a good fit for our solution needed. Unfortunately we were not able to use it as-is because we support iOS from 9 upwards (I don't think many apps support iOS13+ only). So I have changed that. This mostly hit theming, so I tried to use sane defaults (relying on appearance).

Other than that, we also use another library which is out there for a while with the same name MarkdownKit. Therefore I took the liberty to create a namespace to work around this import conflict.

With this fork, we are successfully able to integrate the module. Please let me know your thoughts.

soffes commented 4 years ago

It seems like there are a few big changes here:

I don't want to rename the library. For the deployment target, feel free to PR that separately from the other changes. I think several of the fallbacks from the newer things (like monospace fonts and colors for example) fallback to drastically different things. Picking closer values would be preferred.

Also, just because you deploy to iOS 9, doesn't mean "many apps [don't] support iOS13+ only" :P Even Facebook's deployment target is iOS 10. I'm okay to lower it if it will be easy to maintain, but if it adds a lot of extra friction to new features, I'm not that open to it since that isn't a feature I need. Also, I'm planning on adding macOS support soonish. I'm a little concerned about that plus support for older iOS versions.

Anyway, if you'd like to remove the other stuff from this PR and cleanup the fallbacks, I'm open to reviewing. Hope this all sounds reasonable to you!

josjevv commented 4 years ago

Thanks for your response. Renaming the library is necessary for our usage (or I would have to rename the other one or replace that). Generally I think it's best practice to prefix the library (can be with any prefix, I now just picked your first and last letter), as swift does not have a way do deal with collision at module level. What would be your issue with renaming the library?

I am ok to support iOS 10+, that seems like a sane default. However so far supporting iOS 9 as well requires almost no changes. If that changes in the future, it's easy to bump right? I have no experience with MacOS support, so I have no idea about the impact there.

Most of the compatibility changes are in the DefaultTheme. Right now I had no way to override the theme, that's why I made some changes. If we would create a protocol for a theme, an abstract TextView (that still needs a theme implementation) and an iOS 13 implementation with defaultTheme, you still have your setup while it's reusable for me (and probably others) as well.

It's hard to pick to sane defaults before iOS 13, that's why I fall back to appearance. However, with the protocol based solution anyone can write their implementation (I have now one that implements our theme manager).

Let me know your thoughts on renaming and the protocol based theme setup. If you agree I can make the changes shortly, otherwise it may be difficult for me as my work schedule is quite tight.

soffes commented 4 years ago

Like I said, I'm not open to renaming this. I'm going to close this PR for now. Feel free to reopen or PR the other changes separately if you'd like.