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

Document setState/getState in amp-script #24624

Merged
merged 4 commits into from Oct 2, 2019

Conversation

dreamofabear
Copy link

@dreamofabear dreamofabear commented Sep 19, 2019

Add "Advanced usages" section that explains how to use AMP.setState and AMP.getState in amp-script.

@dreamofabear dreamofabear marked this pull request as ready for review September 19, 2019 15:27
@dreamofabear
Copy link
Author

/to @CrystalOnScript

Copy link
Contributor

@CrystalOnScript CrystalOnScript left a comment

Choose a reason for hiding this comment

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

Just a few questions to help me better understand the documentation scope for this feature :)

@@ -138,6 +138,51 @@ Will be reflected on the page as a new child of the `amp-script` element:

Under the hood, `amp-script` uses [@ampproject/worker-dom](https://github.com/ampproject/worker-dom/). For design details, see the ["Intent to Implement" issue](https://github.com/ampproject/amphtml/issues/13471).

### Advanced usages
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there other "advanced usages" coming? Are we unsupportive of this use case or is this a common request?

Copy link
Author

Choose a reason for hiding this comment

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

There will most likely be more "advanced usages" in the future.

Right now, we think this is an interesting/unique capability of amp-script but it still needs more thought on how it can distill into concrete use cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote for dropping "advanced usages" and titling this section as "State manipulation" or something similar.

Copy link
Author

Choose a reason for hiding this comment

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

I wanted to highlight that it's kind of a niche use case since it requires the amp-bind extension to be installed too. There's already a lot of complex stuff in this documentation so being able to gloss over "Advanced usages" section might be useful. But I'll change it if you insist. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think advanced usages is a little misleading in that it makes it sound challenging rather than niche (although, is challenging misleading :P ?) and would make me believe it isn't a proper use case.

And please correct me if I'm wrong, but this use also sounds like a great way to support full page manipulation without wrapping the entire page in amp-script. Which may not be so niche?

Copy link
Author

Choose a reason for hiding this comment

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

Ok changed.

extensions/amp-script/amp-script.md Outdated Show resolved Hide resolved

`amp-script` supports getting and setting [`amp-state`](https://amp.dev/documentation/components/amp-bind/#initializing-state-with-amp-state) JSON via JavaScript.

This enables advanced interactions between `amp-script` and other AMP elements on the page via `amp-bind` [bindings](https://amp.dev/documentation/components/amp-bind/#bindings).
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this enable amp-script to interact with elements outside of the <amp-script> tag?

Copy link
Author

Choose a reason for hiding this comment

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

Indirectly yes, by modifying amp-state which can affect elements that have bindings.

Is the current phrasing too vague?

Copy link
Contributor

Choose a reason for hiding this comment

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

After this sentence I would add:
"These elements can be inside or outside the amp-script tag."

Copy link
Author

Choose a reason for hiding this comment

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

There's an edge case here: AMP elements inside amp-script doesn't really work well yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something we're working on? If not, should we advise developers to place AMP elements outside of amp-script?

Copy link
Author

Choose a reason for hiding this comment

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

Added the suggested sentence.

extensions/amp-script/amp-script.md Show resolved Hide resolved
Copy link
Contributor

@CrystalOnScript CrystalOnScript left a comment

Choose a reason for hiding this comment

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

Some clarification requests :)

@dreamofabear
Copy link
Author

@CrystalOnScript Cool to merge now? :)

Copy link
Contributor

@CrystalOnScript CrystalOnScript left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks :)

@dreamofabear dreamofabear merged commit e39d762 into ampproject:master Oct 2, 2019
@dreamofabear dreamofabear deleted the script-state-docs branch October 2, 2019 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants