Adds LinkPreview support to MessageKit #1310
Conversation
bguidolim
changed the title
| @@ -110,6 +110,7 @@ public enum MessageKind { | |||
| case emoji(String) // TextMessageCell | |||
| case audio(AudioItem) // AudioMessageCell | |||
| case contact(ContactItem) // ContactMessageCell | |||
| case linkPreview(LinkItem) // LinkPreviewMessageCell | |||
There was a problem hiding this comment.
Wondering if this should be released with a new major version (i.e 4.0.0) because it adds a new case to a public enum. Switches on enum types must be exhaustive so anyone doing a switch on a MessageKind without a default case (completely legal) will have their code break.
There was a problem hiding this comment.
Yeah, next version should be 4.0.0 anyways as we dropped iOS 9 and iOS 10 which is another breaking change. :)
|
@bguidolim, what about video links like youtube? It would be great to have the option to play video links inline as Apple does with the LPLinkView class. |
|
When can we expect this pull request to be merged ? |
tbh, I never planned this. It can be possible, yes, but this is an effort that I don't want to invest in now. |
|
As far as I can see, you don't actually implement any metadata fetching for a provided URL. This pull request only creates new kind of message cell with an imageView and a couple of labels. That is in my opinion worth really little, considering the fact that someone using this library would still have to do the heavy lifting, which is fetching the metadata. |
Well, this is actually the proposal of the library, we don't deal with any data fetching, only the UI. The source of the date coulbe be from anywhere, how can I know from which source should I fetch? |
|
@andre-3-000 MessageKit as a library only aims to provide the outlets for you to do things like fetching data, etc (see the VISION.md). Otherwise we would be imposing/favouring specific implementations. It's not too difficult to fetch the meta data, in fact Apple provides an API for it on iOS 13+ and there are other libraries that exist on Github to do it on older iOS versions. |
There was a problem hiding this comment.
Looks good to me, nice job! Few things:
- I see you added sample data - perfect, can you post screenshot from example app how it looks in light + dark mode? (we should be testing both modes now)
- Definitely needs some tests to be added - probably "ViewsTests"?
- Please add info to the changelog as a breaking change (there should be one already, just add it to it)
|
@Kaspik Here is a screenshot in dark mode.
|
|
@Kaspik Just updated the PR with the changes requested. Please check if it's ok and let me know if you need anything else. |
Codecov Report
@@ Coverage Diff @@
## development #1310 +/- ##
===============================================
+ Coverage 64.75% 66.34% +1.59%
===============================================
Files 68 72 +4
Lines 3305 3515 +210
===============================================
+ Hits 2140 2332 +192
- Misses 1165 1183 +18
Continue to review full report at Codecov.
|
|
hello, that's all what i want to do, please guild me how to get this update. thanks |
|
@Kaspik Can we merge this into dev now since we're targeting 4.0 as the next release? |
|
I think so. :) |
|
nice, hope it's merged soon 👍 |
What does this implement/fix? Explain your changes.
This PR implements a default cell for Link Preview. It inherits from TextMessage calculator and cell and adds a small view containing the title, teaser text, domain, and a thumbnail image.
Here is an example:
Does this close any currently open issues?
No.
Any relevant logs, error output, etc?
No.
Any other comments?
I'm not sure about the implementation itself. This is the first time a calculator and cell inherits a subclass.
I had to set a default font for the LinkPreview view on its calculator. Is it a good place to have it?
I also set a default max-width of 75% of the available view, please check if it is alright.
Where has this been tested?
Devices/Simulators: Simulator - iPhone 11 Pro
iOS Version: 13.4
Swift Version: 5.2
MessageKit Version: 3.1.0