-
Notifications
You must be signed in to change notification settings - Fork 13
fix: sanitize data to match edge delivery markup #244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| const baseUrl = getProductUrl(baseProduct, context); | ||
| if (Array.isArray(option.values)) { | ||
| option.values = option.values.map((value) => ({ | ||
| ...value, |
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.
Was this intentional removal of the spread? Figured you would just append title prop after if that's all you need.
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.
I wanted to make sure that all values are sanitized, and hence removed any "deep copy" / spread.
The template reads only the title and url anyway so that is what I kept.
sirugh
left a comment
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.
I will not be able to validate/test but code looks fine. One comment regarding a change away from spreading of value props, otherwise LGTM.
| if (!html) return html; | ||
|
|
||
| const allowedInlineTags = [ 'a', 'br', 'code', 'del', 'em', 'img', 'strong', 'sub', 'sup', 'u' ]; | ||
| const allowedAllTags = [ |
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.
const allowedAllTags = [
'p', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'ul', 'ol', 'li', 'pre',
'div', 'span', 'section', 'article',
'blockquote', 'cite', 'q',
'figure', 'figcaption',
...allowedInlineTags,
'table', 'tbody', 'td', 'th', 'thead', 'tr',
]; or we don't need all tags ?
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.
We only include what edge delivery supports https://www.aem.live/developer/markup-reference
Fixes #194