✨ Add page sharing options for branching #20777
Conversation
| { | ||
| tag: 'input', | ||
| attrs: dict( | ||
| {'class': 'i-amphtml-page-share-check', |
There was a problem hiding this comment.
nit: { on previous line for consistency
| * starting from a specific page. | ||
| * @private */ | ||
| maybeAddPageShareButton_() { | ||
| if (isExperimentOn(this.win, 'amp-story-branching') && this.isDesktopUi_) { |
There was a problem hiding this comment.
Instead of keeping track of this.isDesktopUi_, would it make sense to just directly call this.storeService_.get(StateProperty.UI_STATE) === UIType.DESKTOP_FULLBLEED here?
There was a problem hiding this comment.
Yes, definitely. Made the change, and did the same with the currentPageId
| .i-amphtml-page-share-check { | ||
| width: 12px !important; | ||
| height: 12px !important; | ||
| vertical-align: middle; |
| const sharePageCheck = | ||
| renderAsElement(this.win.document, SHARE_PAGE_TEMPLATE); | ||
| sharePageCheck.querySelector( | ||
| '#page-share-span').innerHTML = 'Share this page'; |
|
|
||
| const sharePageCheck = | ||
| renderAsElement(this.win.document, SHARE_PAGE_TEMPLATE); | ||
| // sharePageCheck.querySelector( |
There was a problem hiding this comment.
nit: can this leftover commented code be removed?
There was a problem hiding this comment.
Ahh oops. Removed.
| } | ||
|
|
||
| /** | ||
| * @param {boolean} opt_sharePage |
There was a problem hiding this comment.
nit: optional params are specified with an = suffix, e.g. {boolean=}
| */ | ||
| copyUrlToClipboard_(opt_sharePage) { | ||
| const currentPageId = this.storeService_.get(StateProperty.CURRENT_PAGE_ID); | ||
| const url = |
There was a problem hiding this comment.
There's some duplication here. The difference is what's appended, so you can have that be the ternary value
const shouldAddFragment =
(isExperimentOn(this.win, 'amp-story-branching') && opt_sharePage)
const url = Services.documentInfoForDoc(this.getAmpDoc_()).canonicalUrl +
(shouldAddFragment ? '#page=' + currentPageId : '')There was a problem hiding this comment.
This makes sense and is much more readable. Thanks, Carlos!
|
One thing I'm a bit worried about with this feature is the UX we're offering. The sharing and the wording assumes that users know so much about what it's going to do and how our stories work:
That being said, the feature will be very useful for some use cases, especially stories using branching. |
|
/cc @hongwei1990 for @gmajoulet's UX comments above |
|
I think the message here is wrong. What we are doing here is not just share this current page, we are saying your story will share start from this page. The UI is a bit misleading here too. I will take a look. |
|
Please see my UX mocks below |
|
I think this is ready for review! @gmajoulet @newmuis @cvializ |
| uiState === UIType.DESKTOP_FULLBLEED || | ||
| uiState === UIType.DESKTOP_PANELS; | ||
|
|
||
| if (isExperimentOn(this.win, 'amp-story-branching') && isDesktopUi) { |
There was a problem hiding this comment.
I think we can show this even if it's not the desktop UI.
There was a problem hiding this comment.
Hmm, I was in understanding that when on mobile we would be triggering the native sharing UI, of which the design in still pending. Please correct me if I'm wrong!
There was a problem hiding this comment.
Some mobile browsers trigger native sharing, but not all. For those that don't, they'll get this UI!
#20083
In branching, users should also have the ability to share the story from the page they are currently seeing, if they so choose.
Until a better UX solution is found for sharing natively, the ability to share a specific page should exist via the web landscape UI only. When stories support landscape and branching is turned on, users will now have the option to click
Share this pagewhich will generate a url with a page URL fragment parameter (#20238).