-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
✨[amp-social-share][bento][preact] New version of preact social-share component #28137
Changes from 6 commits
09c3686
2baa59a
4474689
a87faf1
3bec731
8c308e1
32e0158
000dfb1
c5ed252
9caade3
5eb95c8
e58dfb0
4e57360
4a2054d
96b90b2
e6609df
e3d162b
b778405
fb01ee0
90de0eb
bd2a7b5
03e4c56
19dd93a
5ad27c2
ac3c294
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -18,9 +18,11 @@ import * as CSS from './social-share.css'; | |||||||
import * as Preact from '../../../src/preact'; | ||||||||
import {Keys} from '../../../src/utils/key-codes'; | ||||||||
import {SocialShareIcon} from '../../../third_party/optimized-svg-icons/social-share-svgs'; | ||||||||
import {addParamsToUrl, parseQueryString} from '../../../src/url'; | ||||||||
import {dict} from '../../../src/utils/object'; | ||||||||
import {getSocialConfig} from './amp-social-share-config'; | ||||||||
import {isObject} from '../../../src/types'; | ||||||||
import {openWindowDialog} from '../../../src/dom'; | ||||||||
import {parseQueryString} from '../../../src/url'; | ||||||||
import {startsWith} from '../../../src/string'; | ||||||||
import {useResourcesNotify} from '../../../src/preact/utils'; | ||||||||
|
||||||||
const DEFAULT_WIDTH = 60; | ||||||||
|
@@ -31,50 +33,91 @@ const DEFAULT_HEIGHT = 44; | |||||||
* @return {PreactDef.Renderable} | ||||||||
*/ | ||||||||
export function SocialShare(props) { | ||||||||
const name = 'SocialShare'; | ||||||||
useResourcesNotify(); | ||||||||
|
||||||||
const { | ||||||||
'typeConfig': typeConfig, | ||||||||
'baseEndpoint': baseEndpoint, | ||||||||
'checkedWidth': checkedWidth, | ||||||||
'checkedHeight': checkedHeight, | ||||||||
} = checkProps(props, name); | ||||||||
const finalEndpoint = createEndpoint(typeConfig, baseEndpoint, props); | ||||||||
|
||||||||
/** | ||||||||
* Handle key presses on the element. | ||||||||
* @param {!Event} event | ||||||||
* @private | ||||||||
* @param {!JsonObject} typeConfig | ||||||||
* @param {?string} baseEndpoint | ||||||||
* @param {!JsonObject} props | ||||||||
* @return {?string} | ||||||||
*/ | ||||||||
function handleKeyPress(event) { | ||||||||
const {key} = event; | ||||||||
if (key == Keys.SPACE || key == Keys.ENTER) { | ||||||||
event.preventDefault(); | ||||||||
handleActivation(); | ||||||||
function createEndpoint(typeConfig, baseEndpoint, props) { | ||||||||
const {params, bindings} = props; | ||||||||
const combinedParams = dict(); | ||||||||
Object.assign(combinedParams, typeConfig['defaultParams'], params); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the linter complain using this form?
Suggested change
|
||||||||
const endpointWithParams = addParamsToUrl(baseEndpoint, combinedParams); | ||||||||
|
||||||||
const combinedBindings = dict(); | ||||||||
const bindingVars = typeConfig['bindings']; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you described and give examples of what params are and what bindings are? So far I'm still really confused about these. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is a summary of how the params and bindings work: https://docs.google.com/document/d/1mtZu3NbD4tgmXtOTtYD_lEvMFUgebEyE4ZJc1ret-Wg/edit#heading=h.ia142csj1140 Params are an object. Each entry in the object will be appended to the shareEndpoint, usually as searchParams (dependent on the type). So if params is Bindings allows variables to be used in the endpoint and params. So if the bindings are {recipient: "testuser} and the endpoint is "endpoint?url=RECIPIENT&userid=3" then the component would process it out to "endpoint?url=testuser&userid=3" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This section takes the logic from the current amp version of the component here: https://github.com/ampproject/amphtml/blob/master/extensions/amp-social-share/0.2/amp-social-share.js#L144 And in preact we can do it synchronously. My thought is that the amp component can create the bindings object (may be async) and pass it to the preact component, then the preact component can handle the logic of creating the endpoint with params and bindings processed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are examples of the bindings right? https://github.com/ampproject/amphtml/blob/master/spec/amp-var-substitutions.md Are they useful outside of an AMP document? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, so the next step for us to figure out what the params + bindings look like in the call to the Preact component. We can always convert the bindings/params as AMP ecosystem needs them. But on the Preact level on of them seems not needed, and the other may need to be passed as props sometimes. E.g. that specific example, a more natural call is:
or at least:
I'd really like to figure out how to just do props, at this point. And to also stress: our current config is AMP specific config. It either needs to change significantly or split into two parts: Preact-vs-AMP. E.g. the fact that we bind AMP's "url" prop to the For instance, the config could look like this:
Thus, the rendering here would be trivial:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That does leave us with the question of what we should do with a custom endpoint. So we just need to see what's the most natural for it. And, IMHO, we can just pass the whole complete There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see your point. And we definitely can change this to accept everything through props. The config model you described has the benefit of making the logic within the Preact component simpler by removing the binding logic, and tradeoff the following:
I guess the question is, is simplicity of the component more desirable vs. more functionality. Another option could be to wrap this component with another Preact component that used the above described new config model. Then the user could choose to use the higher level component which is simpler to use, or use the lower level one if additional granularity is needed (i.e. adding additional custom params). Regarding how the amp component will pass in the parameters, I am not sure yet how this will work. How do you feel about this proposal?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's fine to merge this version and iterate. But to answer the question of additional params - that's easy enough to add with a |
||||||||
if (bindingVars) { | ||||||||
bindingVars.forEach((name) => { | ||||||||
combinedBindings[name.toUpperCase()] = params[name] || ''; | ||||||||
}); | ||||||||
} | ||||||||
if (bindings) { | ||||||||
Object.keys(bindings).forEach((name) => { | ||||||||
combinedBindings[name.toUpperCase()] = bindings[name] || ''; | ||||||||
}); | ||||||||
} | ||||||||
const finalEndpoint = Object.keys(combinedBindings).reduce( | ||||||||
(endpoint, binding) => | ||||||||
endpoint.replace(new RegExp(binding, 'g'), combinedBindings[binding]), | ||||||||
dvoytenko marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
endpointWithParams | ||||||||
); | ||||||||
return finalEndpoint; | ||||||||
} | ||||||||
|
||||||||
/** @private */ | ||||||||
/** | ||||||||
* @private | ||||||||
*/ | ||||||||
function handleActivation() { | ||||||||
const href = props['href']; | ||||||||
const target = props['target'] || '_blank'; | ||||||||
if (!href) { | ||||||||
throw new Error('Clicked before href is set.'); | ||||||||
} | ||||||||
if (startsWith(href, 'navigator-share:')) { | ||||||||
if (!navigator.share) { | ||||||||
throw new Error('no navigator.share'); | ||||||||
if (finalEndpoint.split(':', 1)[0] === 'navigator-share') { | ||||||||
if (window && window.navigator && window.navigator.share) { | ||||||||
const dataStr = finalEndpoint.substr(finalEndpoint.indexOf('?')); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be a util for this in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like there is getFragment() which gets params after '#' symbol but nothing for '?' unless I'm missing somehting. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though I'm not strictly against doing URL splitting here directly. But then we need to handle edge-cases, such as what if there's also a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the calling this out. I was only able to find parseUrlDeprecated which I am hesitant to use because of the Deprecation. I have attempted to write my own helper function (getQueryString, line 192), which handles the corner cases that I can think of.
|
||||||||
const data = parseQueryString(dataStr); | ||||||||
window.navigator.share(data).catch((e) => { | ||||||||
throwWarning(`${e.message}. ${name}`); | ||||||||
}); | ||||||||
} else { | ||||||||
throwWarning( | ||||||||
`Could not complete system share. Navigator unavailable. ${name}` | ||||||||
); | ||||||||
} | ||||||||
const dataStr = href.substr(href.indexOf('?')); | ||||||||
const data = parseQueryString(dataStr); | ||||||||
navigator.share(data).catch(() => { | ||||||||
// TODO(alanorozco): Warn here somehow. | ||||||||
// warn(TAG, e.message, dataStr); | ||||||||
}); | ||||||||
} else { | ||||||||
const windowFeatures = 'resizable,scrollbars,width=640,height=480'; | ||||||||
openWindowDialog(window, href, target, windowFeatures); | ||||||||
openWindowDialog(window, finalEndpoint, '_blank', windowFeatures); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
const type = props['type'].toUpperCase(); | ||||||||
/** | ||||||||
* @private | ||||||||
* @param {!Event} event | ||||||||
*/ | ||||||||
function handleKeyPress(event) { | ||||||||
const {key} = event; | ||||||||
if (key == Keys.SPACE || key == Keys.ENTER) { | ||||||||
event.preventDefault(); | ||||||||
handleActivation()); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
const type = props.type.toUpperCase(); | ||||||||
const baseStyle = CSS.BASE_STYLE; | ||||||||
const backgroundStyle = CSS[type]; | ||||||||
const backgroundStyle = CSS[type] || CSS.DEFAULT; | ||||||||
const size = { | ||||||||
width: props['width'] || DEFAULT_WIDTH, | ||||||||
height: props['height'] || DEFAULT_HEIGHT, | ||||||||
width: checkedWidth, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's floor these instead of checking if they're integers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||||||||
height: checkedHeight, | ||||||||
}; | ||||||||
useResourcesNotify(); | ||||||||
return ( | ||||||||
<div | ||||||||
role="button" | ||||||||
|
@@ -91,3 +134,64 @@ export function SocialShare(props) { | |||||||
</div> | ||||||||
); | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* @param {!JsonObject} props | ||||||||
* @param {?string} name | ||||||||
* @return {!JsonObject} | ||||||||
*/ | ||||||||
function checkProps(props, name) { | ||||||||
const {type, shareEndpoint, params, bindings, width, height} = props; | ||||||||
|
||||||||
// Verify type is provided | ||||||||
if (type === undefined) { | ||||||||
throw new Error(`The type attribute is required. ${name}`); | ||||||||
} | ||||||||
|
||||||||
// bindings and params props must be objects | ||||||||
if (params && !isObject(params)) { | ||||||||
throw new Error(`The params property should be an object. ${name}`); | ||||||||
} | ||||||||
if (bindings && !isObject(bindings)) { | ||||||||
throw new Error(`The bindings property should be an object. ${name}`); | ||||||||
} | ||||||||
|
||||||||
// User must provide shareEndpoint if they choose a type that is not | ||||||||
// pre-configured | ||||||||
const typeConfig = getSocialConfig(type) || dict(); | ||||||||
const baseEndpoint = typeConfig['shareEndpoint'] || shareEndpoint; | ||||||||
if (baseEndpoint === undefined) { | ||||||||
throw new Error( | ||||||||
`A shareEndpoint is required if not using a pre-configured type. ${name}` | ||||||||
); | ||||||||
} | ||||||||
|
||||||||
// Verify width and height are valid integers | ||||||||
let checkedWidth = Math.floor(width || DEFAULT_WIDTH); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an interesting case: Do we ever expect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Edit: Just saw the positive integer check below, in which case even if it is tldr supernit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh thanks for the input. Right, I agree there are many tricky corner cases for this. I've included your input in the new version which now follows the below behavior! // Verify width and height are valid integers |
||||||||
let checkedHeight = Math.floor(height || DEFAULT_HEIGHT); | ||||||||
if (!(checkedWidth > 0)) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I've re-written this seciton, although kept this part with |
||||||||
throwWarning( | ||||||||
`The width property should be a positive integer of type Integer or String, defaulting to ${DEFAULT_WIDTH}. ${name}` | ||||||||
); | ||||||||
checkedWidth = DEFAULT_WIDTH; | ||||||||
} | ||||||||
if (!(checkedHeight > 0)) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||
throwWarning( | ||||||||
`The height property should be a positive integer of type Integer or String, defaulting to ${DEFAULT_HEIGHT}. ${name}` | ||||||||
); | ||||||||
checkedHeight = DEFAULT_HEIGHT; | ||||||||
} | ||||||||
return { | ||||||||
typeConfig, | ||||||||
baseEndpoint, | ||||||||
checkedWidth, | ||||||||
checkedHeight, | ||||||||
}; | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* @param {?string} message | ||||||||
*/ | ||||||||
function throwWarning(message) { | ||||||||
console.warn(message); | ||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
/** | ||
* Copyright 2020 The AMP HTML Authors. All Rights Reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS-IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
import * as Preact from '../../../../src/preact'; | ||
import {SocialShare} from '../social-share'; | ||
import {addParamsToUrl} from '../../../../src/url'; | ||
import {getSocialConfig} from '../amp-social-share-config'; | ||
import {object, select, text, withKnobs} from '@storybook/addon-knobs'; | ||
import {withA11y} from '@storybook/addon-a11y'; | ||
|
||
export default { | ||
title: 'Social Share', | ||
component: SocialShare, | ||
decorators: [withA11y, withKnobs], | ||
}; | ||
|
||
export const _default = () => { | ||
const knobConfigurations = [ | ||
'email', | ||
'facebook', | ||
'linkedin', | ||
'pinterest', | ||
'tumblr', | ||
'twitter', | ||
'whatsapp', | ||
'line', | ||
'sms', | ||
'system', | ||
'custom endpoint', | ||
undefined, | ||
'', | ||
'random', | ||
]; | ||
const type = select( | ||
'Provider Type', | ||
knobConfigurations, | ||
knobConfigurations[0] | ||
); | ||
let href = text('shareEndpoint', 'Not Specified'); | ||
|
||
const config = getSocialConfig(type); | ||
if (type !== 'custom endpoint' && type !== 'random' && type) { | ||
href = addParamsToUrl(config.shareEndpoint, config.defaultParams); | ||
} | ||
const params = object('params', {cat: 1, 'subject': 'test'}); | ||
const bindings = object('bindings', { | ||
cat: 1, | ||
'subject': 'test', | ||
'recipient': 'email recipient', | ||
}); | ||
const width = text('width', undefined); | ||
const height = text('height', undefined); | ||
|
||
return ( | ||
<div> | ||
<p> | ||
Click the button below to share this page using the configured provider. | ||
Update the provider using storybook knobs. Choose Provider Type: 'custom | ||
endpoint' to specify your own share endpoint. | ||
</p> | ||
<SocialShare | ||
type={type} | ||
href={href} | ||
shareEndpoint={href} | ||
params={params} | ||
bindings={bindings} | ||
width={width} | ||
height={height} | ||
/> | ||
</div> | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default color is currently white, which is invisible in most cases against a white background (including in storybook). So I felt it made sense to explicitly set some visible color.