Skip to content
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

✨ [Attachment Forms] Create status indicators that display the page attachment form's submission status #36039

Merged
merged 33 commits into from
Sep 24, 2021

Conversation

coreymasanto
Copy link
Contributor

@coreymasanto coreymasanto commented Sep 13, 2021

The amp-form extension allows for the usage of 3 response attributes for rendering the status of a form submission attempt: submitting, submit-success, and submit-error. Using these attributes, we are adding default status indicators to be used for every page attachment form. Publishers have the option to override a default indicator by including an element with the desired response attribute within their form.

submitting state


submit-success state


submit-error state


Full example

#35569

/cc @gmajoulet

@coreymasanto coreymasanto marked this pull request as ready for review September 13, 2021 14:06
@amp-owners-bot
Copy link

amp-owners-bot bot commented Sep 13, 2021

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/_locales/en.json
extensions/amp-story/1.0/amp-story-form.css
extensions/amp-story/1.0/amp-story-form.js
extensions/amp-story/1.0/amp-story-page-attachment.js
extensions/amp-story/1.0/amp-story-page.js
extensions/amp-story/1.0/amp-story.css
extensions/amp-story/1.0/loading-spinner.js
src/service/localization/strings.js

@coreymasanto
Copy link
Contributor Author

Hey @gmajoulet, mind adding yourself as reviewer?

@gmajoulet gmajoulet requested review from gmajoulet and removed request for raxsha September 13, 2021 16:35
@coreymasanto
Copy link
Contributor Author

I've updated the submitting element so that it always scrolls into view when it is shown. I don't do the same for the submit-success and submit-error states, in order to avoid the page attachment scrolling twice in quick succession.

…t doesn't actually solve the issue of scrollIntoView() not being consistently called
…lements and ensure every response element is scrolled into view upon display
…o avoid a bug where scrolling into view while the attachment is closed causes the entire story page to scroll
@coreymasanto
Copy link
Contributor Author

I've updated the submitting element so that it always scrolls into view when it is shown. I don't do the same for the submit-success and submit-error states, in order to avoid the page attachment scrolling twice in quick succession.

I encountered an Android issue where the submitting element was not being scrolled into view whenever one of the form's inputs had focus (note: calling blur() did not help). However, the submit-success and submit-error elements seem to have no issue being scrolled into view. This is potentially because of behavioral differences (e.g., the submitting element is generally only fleetingly shown, it contains an animated icon, it is the first one shown after pressing submit while an input has focus, etc).

So, I've updated the logic to scroll each of these elements into view when they are displayed.

extensions/amp-story/1.0/amp-story-form.css Outdated Show resolved Hide resolved
* added.
* @private
*/
export function setupResponseAttributeElements(drawerEl, formEl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Passing the drawerEl here makes this function impossible to re-use in other contexts.
I think passing win should allow you to do all you need. Given you haven't been able to get scrollIntoView to work for all cases, it'd be OK to drop it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this function so that win is passed, and I updated the scrolling logic so that it is instead set up from within the amp-story-page-attachment.js. I think we can keep the scrolling logic to help prevent our forms from appearing unresponsive.

extensions/amp-story/1.0/amp-story-form.js Outdated Show resolved Hide resolved
@gmajoulet
Copy link
Contributor

@ampproject/wg-performance @coreymasanto can any of you look into why this is adding 0.82kB to the bundle-size? That looks very high for the number of lines this PR has

@jridgewell
Copy link
Contributor

jridgewell commented Sep 24, 2021

That looks very high for the number of lines this PR has

It seems correct to me. You're inlining a CSS file (with long svgs), and the additional JS code.

@gmajoulet gmajoulet enabled auto-merge (squash) September 24, 2021 18:20
@gmajoulet gmajoulet merged commit 4e2ac0c into ampproject:main Sep 24, 2021
@processprocess processprocess added this to In progress in wg-stories Sprint via automation Sep 27, 2021
@processprocess processprocess moved this from In progress to Done in wg-stories Sprint Sep 27, 2021
@coreymasanto coreymasanto deleted the forms/default-attributes branch October 8, 2021 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants