-
Notifications
You must be signed in to change notification settings - Fork 352
Include option to store embed meta as json #237
Conversation
@@ -298,6 +298,10 @@ | |||
success: function(data) { | |||
var html = data && data.html; | |||
|
|||
if (that.options.storeMeta){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you fix indentation here ?
Can you add a default value to false in |
hopefully that works for you. cleaned up the formatting and added a default option of |
Thanks that look great! Could you try to add some tests that check the json script tag is here when the option is enabled? |
I have this as part of an angular app, so here's the first cut of code I'm using right now when the user submits:
I haven't used Jasmine really at all before, and this is failing below. I will spend a few moments figuring this out later but if you care to shed some light it would save me some time--
|
just for the record, an unescaped </script> tag is going to be parsed in a script element as the closing script tag http://stackoverflow.com/questions/66837/when-is-a-cdata-section-necessary-within-a-script-tag/1450633#1450633.
|
Don't mind adding the regex at all, but I'm not entirely sure it's necessary. My understanding/experience with stringify has always been that the native function handles all the encodings when the workflow is I've ran a several tests using stringify, and I haven't got in trouble yet... thoughts? |
@aggied I've forgotten about this PR a little. How are the tests going? We gonna need them when we want to introduce a new feature. |
Anything I can do to get this accepted? |
I have looked a bit at it today, and it seems like most (perhaps all) of your embed tests use oembedProxy false, and thus mainly tests the 'mock'. Is testing via Embeds.prototype.parseUrl enough? |
Created a new pull request with a merge and some simple test, let me know if you need anything else, Store meta #333 |
Merged with #333 |
Picking up projects that use this repo again. @linpekka thanks for finishing this off |
Np, you did all the real stuff, im just the cleaner. |
I want to resolve #235 by accepting a
storeMeta
option in the embed config that when set totrue
stores embed json in a<script>
tag in the resulting html.example embed config: