fix: custom validation refactor #11
Conversation
demo/index.html
Outdated
| amount: '500', | ||
| style: { | ||
| layout: 'custom', | ||
| markup: 'https://localhost.paypal.com:8080/custom.html' |
There was a problem hiding this comment.
We should probably refrain from committing changes to the demo pages, especially with for non-general use-cases.
There was a problem hiding this comment.
Yeah I'm gonna revert this back. For now though I wanted Ryan to have an easily accessible demo page to test the story.
| return validated ? markup : ''; | ||
| }) | ||
| ); | ||
| return markupProm.then(markup => markup); |
There was a problem hiding this comment.
It looks like we are accepting any style.markup value. We should probably ensure that a URL is used from paypalobjects or give a blank message.
There was a problem hiding this comment.
I changed the regex to match htt[s://www.paypalobjects.com
demo/custom.html
Outdated
| <!-- | ||
| {"foo":"bar","style":{"ratio":"2x1"}} | ||
| --> | ||
| <style> |
There was a problem hiding this comment.
Do we want an example of this in our repo?
There was a problem hiding this comment.
I think this can get reverted back along with the demo page.
src/models/Banner/index.js
Outdated
| partial(objectAssign, { options }), // Object(markup, options) | ||
| insertMarkup // Promise<Object(meta)> | ||
| ) | ||
| insertMarkup // Promise<Object(meta)> |
There was a problem hiding this comment.
| insertMarkup // Promise<Object(meta)> | |
| insertMarkup // Promise<Object(meta, options)> |
src/models/Banner/index.js
Outdated
| @@ -56,16 +56,13 @@ const Banner = { | |||
|
|
|||
| function render(totalOptions) { | |||
| const options = validateOptions(totalOptions); | |||
| const renderProm = getBannerMarkup(options) // Promise<Object(markup)> | |||
| const renderProm = getBannerMarkup(options) // Promise<Object({ markup, options })> | |||
There was a problem hiding this comment.
| const renderProm = getBannerMarkup(options) // Promise<Object({ markup, options })> | |
| const renderProm = getBannerMarkup(options) // Promise<Object(markup, options)> |
src/services/banner/index.js
Outdated
|
|
||
| return data; | ||
| const bannerOptions = getBannerOptions(template); | ||
| options = objectMerge(options, { ...bannerOptions }); |
There was a problem hiding this comment.
Consider creating a new variable instead of reassigning the options reference.
Seavenly
changed the title
src/services/banner/index.js
Outdated
| function getBannerOptions(markup) { | ||
| const annotationsString = markup.match(/^<!--([\s\S]+?)-->/); | ||
| if (annotationsString) { | ||
| return JSON.parse(annotationsString[1]); |
There was a problem hiding this comment.
Would it be valuable to wrap this in a try { } catch { } in case the contents of the comment are not valid JSON?
src/services/banner/index.js
Outdated
|
|
||
| return data; | ||
| const bannerOptions = getBannerOptions(template); | ||
| options = objectMerge(options, { ...bannerOptions }); |
There was a problem hiding this comment.
Can bannerOptions be passed as-is as the second argument here?
| return validated ? markup : ''; | ||
| }) | ||
| ); | ||
| return markupProm.then(markup => markup); |
There was a problem hiding this comment.
This can just be return markupProm, or could just return directly without declaring markupProm
src/services/banner/index.js
Outdated
| return JSON.parse(annotationsString[1]); | ||
| try { | ||
| const bannerOptions = JSON.parse(annotationsString[1]); | ||
| return bannerOptions; |
There was a problem hiding this comment.
Can't we just return JSON.parse(annotationsString[1])?
There was a problem hiding this comment.
Yup - remnant of some previous code.
| @@ -22,9 +22,12 @@ function fetcher(url) { | |||
| } | |||
|
|
|||
| function getCustomTemplate(styles) { | |||
| let markupRegex = /https:\/\/www\.paypalobjects\.com/; | |||
There was a problem hiding this comment.
Is it feasible to use String.prototype.startsWith() instead of a regex for this scenario? And for __DEMO__ we can just accept anything since it's for internal use only?
There was a problem hiding this comment.
Also, have we considered how this will work with the customization app since everything is now some form of URL?
No description provided.