Allow rendering response data in the form success/error messages#3583
Allow rendering response data in the form success/error messages#3583mkhatib merged 6 commits intoampproject:masterfrom
Conversation
|
@dvoytenko should do a quick pass for mustache integration. |
extensions/amp-form/0.1/amp-form.js
Outdated
There was a problem hiding this comment.
Why is this in a then block? Can't it be done sync?
There was a problem hiding this comment.
I found this work better for testing. I don't really understand exactly why but basically If I rethrowAsync sync after the this.renderTemplate call, the test always fail with the error being thrown, if I try to expect that test to eventually throw it would catch the error but it doesn't seem to test the last block of the test which does the actual expectations inside the timer.promise(0).then.
I can try and debug this some more if you think we should do this sync.
There was a problem hiding this comment.
Likely because you are "catching" the UnhandledRejection when thrown async.
There was a problem hiding this comment.
How do we avoid this? I might need to re-write the way I am testing this. Will try to do some more tests here.
There was a problem hiding this comment.
Fixed this by changing timer.promise(20) to timer.promise(0) in the test.
|
PTAL |
examples/forms.amp.html
Outdated
There was a problem hiding this comment.
If we introduce submit-error and such attributes - we have to configure CSS for them in our CSS, right? Otherwise, it'd be illogical to whitelist these attributes in validator but not give them any default meaning.
There was a problem hiding this comment.
Yes I think we should. I think it makes sense in these two cases to manage display for the user as well. So basically moving these styles to be provided by amp by default. Des that make sense?
There was a problem hiding this comment.
Yes, specifically form [submit-success] and form [submit-error], right?
There was a problem hiding this comment.
Should I move the styles into amp-form.css or these should be part of amp.css? Should I do it in the PR? I think the initial display:none needs to go in the amp.css, no?
There was a problem hiding this comment.
At the very least, form [..] {display: none} should be in amp.css to avoid FOUC.
There was a problem hiding this comment.
Done.
Tracking adding display:block to these containers in amp-form.css in the ITI PR. Will add in next PRs.
|
So, this is LGTM. Just one request to fallback |
src/service/xhr-impl.js
Outdated
There was a problem hiding this comment.
(You'll probably move this to a separate PR, but...) Before attempting to do json() here, we need to check that the Content-Type is actually JSON. The error response doesn't have to follow the same protocol as the successful response and typically doesn't do so.
ITI: #3343