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

Introduce a method to update localstorage value #19186

Merged
merged 2 commits into from Nov 10, 2018

Conversation

zhouyx
Copy link
Contributor

@zhouyx zhouyx commented Nov 8, 2018

This is necessary for #17742 to support forcePromptNext.

In order to support that I'll need to write a dirty bit to the localStorage in order to erase the value next time. I need to erase storage next time, because the current stored consent string will be required for UI the next time.

However in order to be GDPR compliance, the current stored value need to be expired based on the last user consent timestamp. Which requires me to update the local stored value w/o changing the timestamp.

Copy link
Contributor

@lannka lannka left a comment

Choose a reason for hiding this comment

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

LGTM, might need some more JS doc explaining opt_isUpdate and its typical use case.
now we kinda redefine timestamp as creationTimestamp.

@zhouyx
Copy link
Contributor Author

zhouyx commented Nov 9, 2018

Added comment. As discussed offline I decided not to introduce new update API because there will be update and updateNonBoolean.

@zhouyx zhouyx merged commit 8169cbc into ampproject:master Nov 10, 2018
Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants