MilanLund / kentico-cloud-delivery-js-sdk

Unofficial SDK for the Delivery part of Kentico Cloud
MIT License
0 stars 4 forks source link

Modular content issue #2

Closed martinabrahams closed 3 years ago

martinabrahams commented 6 years ago

Hi Milan, I've ran into a strange issue using modular content. After debugging I've found that the resolveModularContentInRichText() function isn't able to match the modular content based on the supplied modularContentCodeName.

I'm specifying the modularContentCodeName value as the Kentico Cloud content type and a custom template to suit that content type.

What I found was happening is that resolveModularContentInRichText() is matching modular content based on , but in the raw response data from the delivery API, this codename is actually the system.codename for the record (instance specific, not type specific), so it never matches anything.

So I have two theories. 1 - Kentico has changed the format 2- I'm using it completely wrong.

Both are very possible.

I've re-written the function in my fork and I have got it working, but I won't put in a PR without some clarification from you.

Thanks Marty

MilanLund commented 6 years ago

Hi Marty,

Thanks for the notification. The resolveModularContentInRichText function isn't designed well, I know. Sorry for making you confused. The SDK is one of my side projects, I work on them in my free time and they could have such issues. When I have time I will take a look at your implementation. Kentico Software and I are currently discussing an opportunity to create an official Node.js SDK. That would allow me to make it well. I would also appreciate your feedback on the current SDK... Wondering if the idea behind it is relevant to you.

Cheers,

Milan Lund Freelance Web Developer for Kentico projects www.milanlund.com

2017-10-25 5:51 GMT+02:00 Martin Abrahams notifications@github.com:

Hi Milan, I've ran into a strange issue using modular content. After debugging I've found that the resolveModularContentInRichText() function isn't able to match the modular content based on the supplied modularContentCodeName.

I'm specifying the modularContentCodeName value as the Kentico Cloud content type and a custom template to suit that content type.

What I found was happening is that resolveModularContentInRichText() is matching modular content based on , but in the raw response data from the delivery API, this codename is actually the system.codename for the record (instance specific, not type specific), so it never matches anything.

So I have two theories. 1 - Kentico has changed the format 2- I'm using it completely wrong.

Both are very possible.

I've re-written the function in my fork and I have got it working, but I won't put in a PR without some clarification from you.

Thanks Marty

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/MilanLund/kentico-cloud-delivery-js-sdk/issues/2, or mute the thread https://github.com/notifications/unsubscribe-auth/ACDAMJG-Z24xjSI2RKHlu3juzqwDEsRCks5svrAsgaJpZM4QFZmd .

martinabrahams commented 6 years ago

Hi Milan, Thanks for getting back to me. Honestly this SDK is fine for my project, it's a really big help and I appreciate you taking the time to dev it.

In my project I have a parent content type represents an article, it has a rich text field to hold the article content. The rich text field accepts different types of modular content such as a video, block quote, featured tweet etc so that they can be inserted amongst the text content. I'd say this is a text book usage of modular content.

The only thing that makes this project different is that it's running in nodejs, not the browser, but that can't be a factor in this scenario. I tried the TypeScript SDK, but it doesn't support nodejs due to the way they handle HTTP requests.

The issue seems like a bug to me, I spent a day trying to get it work then after reverse engineering your code I found that it locates the by the instance identifier, not the content type codename.

I also found that it would not handle multiple instances of the same content type, it would only resolve the first instance found in the rich text field. So I've refactored it to iterate through each object result.

I need to persist with my fork one way or another as I'm using this in a real project, but I'd prefer to assist you with bug fixes.

Cheers Marty

martinabrahams commented 6 years ago

Hi Milan, After reviewing your unit tests, I think I have found the issue and/or the cause of my misunderstanding.

Your test data = project.resolveModularContentInRichText(data, 'testItemsParent', 'rich_text', 'rich_text_modular_item', '<span class="resolved">{elements.text}</span>')

Your documentation states:

modularContentCodeName Code name of a modular item that is inside of the Rich text element.

This is exactly what your code is doing, looking up by the codename. So technically this isn't a bug.

However my issue is that how would you ever know what the code name is? The codename is instance specific.

For my usage, it's the content type that matters. I want to say for this content type, format with this template. Not for the article with the codename "my_first_article_2017", format it with this template.

Based off your unit tests and data, if I change the test to use the type, I get the error that I encountered.

data = project.resolveModularContentInRichText(data, 'testItemsParent', 'rich_text', 'automated_test_item', '<span class="resolved">{elements.text}</span>')

In my fork, I changed the test to above and it works.

So I think maybe the answer is to there should be two resolve methods, one by type and one by codename?

I personally can't see any use for the codename scenario but maybe this is where I'm misunderstanding something.

Cheers Marty

MilanLund commented 6 years ago

Hi Marty,

You described the wrong design of the resolveModularContentInRichText method well. The method is intended to resolve one specific modular content in a rich text field. Technically, it made sense to have such method when I was developing the SDK. However, when I was testing the SDK on a real project the method haven't come in handy because of the reason you describe. In most cases, you don't know codename of the modular content. It would make more sense to have a method that resolves modular content by type.

The problem I had is that modular content in a rich text field gets evaluated as the object tag: <object type=\"application/kenticocloud\" data-type=\"item\" data-codename=\"css\">

As you can see there are data-type and data-codename attributes. The data-type attribute seems to contain "item" no matter what content type it is. So the only option is to look for matching codenames in the modular_content object. This is the reason I designed the method to require the codename.

If we need to get the modular content by type I guess I would have to iterate the modular_content object first and find items that match the specified content type. Then I would have to find matching object tags in the rich text field. Regrettably, I haven't thought of such scenario when I was working on the SDK.

I have it on my to-do list. Currently, I am not sure when I get to that. As I mentioned before, I will probably contribute the official Node.js SDK so this is something we should address. I appreciate you share your experience and needs!

Cheers,

Milan Lund Freelance Web Developer for Kentico projects www.milanlund.com

2017-10-27 2:07 GMT+02:00 Martin Abrahams notifications@github.com:

Hi Milan, After reviewing your unit tests, I think I have found the issue and/or the cause of my misunderstanding.

Your test data = project.resolveModularContentInRichText(data, 'testItemsParent', 'rich_text', 'rich_text_modular_item', '{elements. text}')

Your documentation states:

modularContentCodeName Code name of a modular item that is inside of the Rich text element.

This is exactly what your code is doing, looking up by the codename. So technically this isn't a bug.

However my issue is that how would you ever know what the code name is? The codename is instance specific.

For my usage, it's the content type that matters. I want to say for this content type, format with this template. Not for the article with the codename "my_first_article_2017", format it with this template.

Based off your unit tests and data, if I change the test to use the type, I get the error that I encountered.

data = project.resolveModularContentInRichText(data, 'testItemsParent', 'rich_text', 'automated_test_item', '{elements. text}')

In my fork, I changed the test to above and it works.

So I think maybe the answer is to there should be two resolve methods, one by type and one by codename?

I personally can't see any use for the codename scenario but maybe this is where I'm misunderstanding something.

Cheers Marty

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MilanLund/kentico-cloud-delivery-js-sdk/issues/2#issuecomment-339835183, or mute the thread https://github.com/notifications/unsubscribe-auth/ACDAMNqck2H_Ia0Q5zIgDn2r4aPQcST0ks5swR61gaJpZM4QFZmd .

martinabrahams commented 6 years ago

Hi Milan, Ok it makes alot of sense now. I have actually implemented this last week - https://github.com/BanjoAdvertising/banjo-kentico-cloud-delivery-js-sdk/commit/45b6addf6b9b9556042c7495882eeff24fbc4da0#diff-1fdf421c05c1140f6d71444ea2b27638

I changed the method to look up by content type and it supports multiple instances of the same content type in the one rich text field.

If you'd like I can split it into a seperate method and test case and do a proper PR for it.

Cheers Marty

MilanLund commented 6 years ago

Hi Marty,

that would be awesome!

Thanks,

Milan Lund Freelance Web Developer for Kentico projects www.milanlund.com

2017-10-29 23:12 GMT+01:00 Martin Abrahams notifications@github.com:

Hi Milan, Ok it makes alot of sense now. I have actually implemented this last week

I changed the method to look up by content type and it supports multiple instances of the same content type in the one rich text field.

If you'd like I can split it into a seperate method and test case and do a proper PR for it.

Cheers Marty

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MilanLund/kentico-cloud-delivery-js-sdk/issues/2#issuecomment-340306652, or mute the thread https://github.com/notifications/unsubscribe-auth/ACDAMHXwq5wLmPfl0bZfXJdsBwYJ5Jz-ks5sxPhCgaJpZM4QFZmd .

Enngage commented 6 years ago

Hi guys,

I noticed you tagged my issue regarding node.js support. Yesterday I released a new version of SDK that works in node.js environment by using the built-in HTTP client that wraps callback results in Observables. It is still in Beta so I would be happy to hear any feedback. ¨

martinabrahams commented 6 years ago

Hi Engage, I am keen to give this a try, thanks for letting me know