Skip to content

Update form-vars-sub to use data-amp-replace and value attrs#6621

Merged
mkhatib merged 4 commits intoampproject:masterfrom
mkhatib:form-vars-sub-changes
Dec 14, 2016
Merged

Update form-vars-sub to use data-amp-replace and value attrs#6621
mkhatib merged 4 commits intoampproject:masterfrom
mkhatib:form-vars-sub-changes

Conversation

@mkhatib
Copy link
Copy Markdown
Contributor

@mkhatib mkhatib commented Dec 13, 2016

A follow up to this discussion.

Let me know if this looks good, will update tests.

Copy link
Copy Markdown
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tests, please.

* @param {!HTMLInputElement} element
* @return {string} Replaced string for testing
*/
expandInputValueSync(element) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor to remove code duplication with Async function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@mkhatib
Copy link
Copy Markdown
Contributor Author

mkhatib commented Dec 14, 2016

PTAL 👀

@mkhatib mkhatib merged commit e50312b into ampproject:master Dec 14, 2016
@mkhatib mkhatib deleted the form-vars-sub-changes branch December 14, 2016 22:08
}
const requestedReplacements = {};
whitelist.trim().split(/\s+/).forEach(replacement => {
if (!opt_supportedReplacement ||
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is useless?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. this can be dropped. 👍

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants