✨ [Bento] Implement bento-app-banner
#37127
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hey @ampproject/wg-caching! These files were changed: Hey @jridgewell! These files were changed: |
added 6 commits
December 6, 2021 15:35
dethstrobe
reviewed
Dec 7, 2021
|
@jridgewell Yes, ready to re-review. The "document-scoped services" will be implemented separately from this PR (#37465), as to prevent even more bloat in this PR |
dethstrobe
reviewed
Jan 26, 2022
dethstrobe
reviewed
Jan 26, 2022
rcebulko
reviewed
Jan 27, 2022
added 4 commits
January 27, 2022 10:51
jridgewell
reviewed
Jan 27, 2022
| * @return {boolean} | ||
| */ | ||
| isFirefox() { | ||
| return /Firefox|FxiOS/i.test(self.navigator.userAgent) && !this.isEdge(); |
There was a problem hiding this comment.
Actually, terser can minify object values but not method shorthands. Converting this to isFirefox: () => { /* … */ } and we don't need to worry.
But, I also like the idea of making this module level functions, which you can then import * as platform from 'platform.js' and use just like the old platform service object.
| const metas = self.document.head.querySelectorAll('meta[name]'); | ||
| return ( | ||
| Array.from(metas.values()) | ||
| .find( |
There was a problem hiding this comment.
Nit: this is my preference, but I hate using array methods when something is easily expressed as a for-loop.
There was a problem hiding this comment.
#37127 (comment)
I would like to reduce the number of "preferences" blocking this PR.
There was a problem hiding this comment.
@jridgewell this is on me, I wrote/suggested this code in place of an existing for-loop. Opposite preferences 😬
There was a problem hiding this comment.
Nits are never blocking.
Co-authored-by: Justin Ridgewell <[email protected]>
jridgewell
approved these changes
Jan 28, 2022
| const metas = self.document.head.querySelectorAll('meta[name]'); | ||
| return ( | ||
| Array.from(metas.values()) | ||
| .find( |
There was a problem hiding this comment.
Nits are never blocking.
samouri
pushed a commit
to samouri/amphtml
that referenced
this pull request
Feb 2, 2022
* feature(bento-add-banner): initial template generation * feature(bento-add-banner): created simple storybook * feature(bento-add-banner): copied CSS from classic * feature(bento-add-banner): added many "stubs" for standalone services * feature(bento-add-banner): ported IOS and Android app logic * feature(bento-add-banner): added platform-aware logic, and dismiss button storage * feature(bento-add-banner): added UI unit tests * feature(bento-add-banner): added IOS tests * feature(bento-add-banner): moved sources into `component` folder * feature(bento-add-banner): added unit tests for Android * feature(bento-add-banner): mapped the `dismiss-button-aria-label` attribute * feature(bento-add-banner): added minor notes to the stories * feature(bento-add-banner): added unit tests for amp-app-banner * feature(bento-add-banner): updated validator * feature(bento-add-banner): extracted services to common location * feature(bento-add-banner): added README for services * feature(bento-add-banner): updated type defs * feature(bento-add-banner): updated formatting * feature(bento-add-banner): lint fix * feature(bento-add-banner): lint ignores * feature(bento-add-banner): lint fixes * feature(bento-add-banner): type fixes * feature(bento-add-banner): updated z-index file * feature(bento-add-banner): updated validator template * feature(bento-add-banner): simplified code with `Array.find` * feature(bento-add-banner): parse meta content using `Array.reduce` * feature(bento-add-banner): added more tests for android * feature(bento-add-banner): added more tests for ios * feature(bento-add-banner): improved tests for BentoAppBanner * feature(bento-add-banner): added tests for Dismiss logic * feature(bento-add-banner): added more iOS tests * feature(bento-add-banner): tiny lint fix * feature(bento-add-banner): improved tests for web-component mode, and fixed the button[open-button] check * feature(bento-add-banner): removed unnecessary `waitFor` * feature(bento-add-banner): lint fix * feature(bento-add-banner): added whitespace in tests * feature(bento-add-banner): ensure tests are not dependent on timers * feature(bento-add-banner): removed obsolete `latestVersion` * feature(bento-add-banner): use `Object.fromEntries` as suggested * feature(bento-add-banner): removed unused CSS * feature(bento-add-banner): use `WindowInterface.getTop` to improve unit tests * feature(bento-add-banner): extracted querySelectorInSlot to core * feature(bento-add-banner): removed useless services, use object-syntax instead of classes, fixed fetchJson * feature(bento-add-banner): renamed services to utils * feature(bento-add-banner): removed redundant win variable * feature(bento-add-banner): extracted parsing logic into separate function for readability * feature(bento-add-banner): renamed file to `docInfo` * feature(bento-add-banner): improved safety of accessing localStorage * feature(bento-add-banner): ensure proper default value is returned * feature(bento-add-banner): removed unnecessary async * feature(bento-add-banner): ensure localStorage still works if `key` changes * feature(bento-add-banner): flip ternary for readability * feature(bento-add-banner): added `self` for fetch * feature(bento-add-banner): updated generated z-index file * feature(bento-app-banner): ensure XHR rejects invalid status codes * feature(bento-app-banner): removed dependencies on `/src/url` * feature(bento-app-banner): lint sample code * feature(bento-app-banner): minor code improvements * feature(bento-app-banner): minor code improvements * feature(bento-app-banner): minor code improvements * feature(bento-app-banner): allow localStorage from custom hook * feature(bento-app-banner): include optional `init` param for requests * feature(bento-app-banner): improved logging * feature(bento-app-banner): only expose `logger[info|warn|error]` * feature(bento-app-banner): warn when localStorage.setItem fails * feature(bento-app-banner): ensure logger can be stubbed and tested * feature(bento-app-banner): moved `INVALID_PROTOCOLS` to proper location * feature(bento-app-banner): ensure logger is globally disabled * feature(bento-app-banner): ensure component inherits from AmpPreactBaseElement * Update src/preact/utils/docInfo.js Co-authored-by: Justin Ridgewell <[email protected]> Co-authored-by: scottrippey <[email protected]> Co-authored-by: Justin Ridgewell <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
This PR implements the
amp-app-bannercomponent as a standalone Bento component,bento-app-banner.Functionality ported over:
<meta>tag to determine App Store linksmanifest.jsonto determine Play Store linksFunctionality not ported:
Testing
In Amp Storybook:
