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

✨[amp-social-share][bento][preact] New version of preact social-share component #28137

Merged
merged 25 commits into from
May 14, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
09c3686
First draft, new version of preact social-share component
krdwan Apr 30, 2020
2baa59a
Moved contents to social-share and added story
krdwan Apr 30, 2020
4474689
delete old version of social-share2
krdwan Apr 30, 2020
a87faf1
Adding missed change to story
krdwan Apr 30, 2020
3bec731
Revisions from initial code review
krdwan May 1, 2020
8c308e1
renamed private funcitons in amp-social-share
krdwan May 1, 2020
32e0158
Updated storybook and social-share, renamed from 0.2 to 1.0
krdwan May 11, 2020
000dfb1
Adding missed change on amp social share
krdwan May 11, 2020
c5ed252
More changes to preact component
krdwan May 13, 2020
9caade3
Add some minor changes in how props are unpacked
krdwan May 13, 2020
5eb95c8
Removed a typo
krdwan May 13, 2020
e58dfb0
update getQueryString helper
krdwan May 13, 2020
4e57360
Fix a typo
krdwan May 13, 2020
4a2054d
Merge with master
krdwan May 13, 2020
96b90b2
Update to getQueryString helper for use case where a ? exists within …
krdwan May 13, 2020
e6609df
Cleaner syntax in getQueryString helper
krdwan May 13, 2020
e3d162b
Even cleaner more elegant syntax for getQueryString
krdwan May 13, 2020
b778405
Update getQueryString, update versioning from 0.2 to 1.0
krdwan May 13, 2020
fb01ee0
Fixed linter issue, update struct definition
krdwan May 14, 2020
90de0eb
console /*OK*/.warn to pass linter error (warn in preact env)
krdwan May 14, 2020
bd2a7b5
update config for 0.2 -> 1.0
krdwan May 14, 2020
03e4c56
linter updates
krdwan May 14, 2020
19dd93a
Adding parenthesis around type casts
krdwan May 14, 2020
5ad27c2
Updates for type checking
krdwan May 14, 2020
ac3c294
removed null coalescing operator to pass pre-merge checks
krdwan May 14, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
203 changes: 169 additions & 34 deletions extensions/amp-social-share/0.2/social-share.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -30,63 +32,196 @@ const DEFAULT_HEIGHT = 44;
* @param {!JsonObject} props
* @return {PreactDef.Renderable}
*/
export function SocialShare(props) {
export function SocialShare2(props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

"2"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the name, because I was concerned that I would break the amp component. The old version of the preact component has different behavior, and the amp component relies on the old version. Should I remove the 2?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Let's break.

const name = 'SocialShare2';
useResourcesNotify();

const {
typeConfig,
baseEndpoint,
checkedWidth,
checkedHeight,
obsoleteType,
} = checkProps_(props, name);
if (obsoleteType) {
return;
}
const finalEndpoint = createEndpoint_(typeConfig, baseEndpoint, props);

/**
* Handle key presses on the element.
* @param {!Event} event
* @private
* @param {!JsonObject} props
* @param {?string} name
* @return {!JsonObject}
*/
function handleKeyPress(event) {
const {key} = event;
if (key == Keys.SPACE || key == Keys.ENTER) {
event.preventDefault();
handleActivation();
function checkProps_(props, name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a static function? If so, let's make it a module-level function. Also, let's drop _ for this and others.

const {type, shareEndpoint, params, bindings, width, height} = props;
Copy link
Contributor

Choose a reason for hiding this comment

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

For now, very unfortunately, these would have to be defined with quotes. E.g.

const {'type': type, ...} = props;


// Verify type is valid and cannot contain spaces
if (type === undefined) {
throwError_(`The type attribute is required. ${name}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just do throw new Error() here? If we actually throw errors, it'd be better to be explicit here, since the compiler understands that the function exits with the throw statement.

But, I'm not yet sold we should be throwing errors here. I think they are somewhat unidiomatic in React development. But let's do it for now, until we figure out a better way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I wrapped the actual throwing of an error inside it's own function in case we want to change it throughout the component, since I am not sure what is the idiomatic way to throw an error in React and specifically for bento.

Sure I can make it explicit here.

Copy link
Contributor

Choose a reason for hiding this comment

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

We just need to be consistent here. If it's a method call that may or may not throw in different cases, we have to propagate null/undefined values, and then skip rendering by returning null. If we throw errors and don't want to deal with this yet, we should make throw explicit to compiler and the only way to do it is to actually have throw ... at the call site. I recommend we start by throwing actual errors explicitly - it's less effort for us for now and we'd likely have to pass through the errors again at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, let's throw the error explicitly.

}
if (/\s/.test(type)) {
throwError_(
Copy link
Contributor

Choose a reason for hiding this comment

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

This error seems way to detailed and too obscure. Is there a specific reason why we should worry about spaces here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a carry over from the previous version of social-share. I wanted to keep the behavior the same. I can remove if desired.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed!

`Space characters are not allowed in type attribute value. ${name}`
);
}

// bindings and params props must be objects
if (params && !isObject(params)) {
throwError_(`The params property should be an object. ${name}`);
}
if (bindings && !isObject(bindings)) {
throwError_(`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) {
throwError_(
`A shareEndpoint is required if not using a pre-configured type. ${name}`
);
}

// Throw warning if type is obsolete
const obsoleteType = typeConfig['obsolete'];
Copy link
Member

Choose a reason for hiding this comment

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

What are the conditions that lead to an obsolete type?

Since this is a new version of the component, let's get rid of types we don't support if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's only one obsolete type, it's 'gplus'.

Yes, we can just remove support for this type and remove this functionality. It's included here: https://github.com/ampproject/amphtml/blob/master/extensions/amp-social-share/0.2/amp-social-share-config.js

So wasn't sure if there might be a future need for this type.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's remove :)

if (obsoleteType) {
throwWarning_(`Skipping obsolete share button ${type}. ${name}`);
}

// Verify width and height are valid integers
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we'd separate size and type-config code from each other.

let checkedWidth = width || DEFAULT_WIDTH;
let checkedHeight = height || DEFAULT_HEIGHT;
if (width && !Number.isInteger(width)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-integer width and height are generally totally ok, even if a bit uncommon.

throwWarning_(
`The width property should be an Integer, defaulting to ${DEFAULT_WIDTH}. ${name}`
);
checkedWidth = DEFAULT_WIDTH;
}
if (height && !Number.isInteger(height)) {
throwWarning_(
`The height property should be an Integer, defaulting to ${DEFAULT_HEIGHT}. ${name}`
);
checkedHeight = DEFAULT_HEIGHT;
}
return {
typeConfig,
baseEndpoint,
checkedWidth,
checkedHeight,
obsoleteType,
};
}

/** @private */
function handleActivation() {
const href = props['href'];
const target = props['target'] || '_blank';
if (!href) {
throw new Error('Clicked before href is set.');
/**
* @private
* @param {!JsonObject} typeConfig
* @param {?string} baseEndpoint
* @param {!JsonObject} props
* @return {?string}
*/
function createEndpoint_(typeConfig, baseEndpoint, props) {
const {params, bindings} = props;
const combinedParams = dict();
Object.assign(combinedParams, typeConfig['defaultParams'], params);
Copy link
Member

Choose a reason for hiding this comment

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

Does the linter complain using this form?

Suggested change
const combinedParams = dict();
Object.assign(combinedParams, typeConfig['defaultParams'], params);
const combinedParams = dict({...typeConfig['defaultParams'], ...params});

const endpointWithParams = addParamsToUrl(baseEndpoint, combinedParams);

const combinedBindings = dict();
const bindingVars = typeConfig['bindings'];
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
{url: "google.com", userid: "3"}, the endpoint would look like "endpoint?url=google.com&userid=3" (it will be url encoded as well)

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"

Copy link
Contributor Author

@krdwan krdwan May 1, 2020

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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:

<social-share type="twitter" url="google.com" />

or at least:

<social-share type="twitter" params={{url: 'google.com'}} />

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 CANONICAL_URL, is specifically AMP's view of things, I believe.

For instance, the config could look like this:

const TYPES = {
  LINKEDIN: {
    /* Props. Come up with anything that looks natural to us internally. */
    requiredProps: ['url'],
    shareEndpoint: ({url, mini = true}) => `https://www.linkedin.com/shareArticle?url=${enc(url)&mini=${enc(mini)}}`,
  },
}

Thus, the rendering here would be trivial:

const config = TYPES[type];
if (config) {
  validateProps(props, config);
  endpoint = config. shareEndpoint(props);
}

Copy link
Contributor

Choose a reason for hiding this comment

The 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 shareEndpoint with all params interpolated.

Copy link
Contributor Author

@krdwan krdwan May 11, 2020

Choose a reason for hiding this comment

The 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:

  • Cannot add arbitrary additional parameters to the url (all params per type would need to be pre-defined in the config)
  • For custom url, the user would not be able to rely on our logic to format any parameters to be appended to the end of the url (more logic required from user outside of our component)

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?

  1. Merge the current version of preact component
  2. Work on amp component updates
  3. Understand with regard to both preact and amp what makes the most sense for us to do
  4. Create a separate PR that will include amp component and changes to preact if needed (which may include, the simplified config described above)

Copy link
Contributor

Choose a reason for hiding this comment

The 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 additionalParams prop. My point was that the key parameters should be top-level props. Other things can be tacked on as needed.

if (bindingVars) {
bindingVars.forEach((name) => {
combinedBindings[name.toUpperCase()] = params[name] ? params[name] : '';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
combinedBindings[name.toUpperCase()] = params[name] ? params[name] : '';
combinedBindings[name.toUpperCase()] = params[name] ?? '';

});
}
if (startsWith(href, 'navigator-share:')) {
if (!navigator.share) {
throw new Error('no navigator.share');
}
const dataStr = href.substr(href.indexOf('?'));
const data = parseQueryString(dataStr);
navigator.share(data).catch(() => {
// TODO(alanorozco): Warn here somehow.
// warn(TAG, e.message, dataStr);
if (bindings) {
Object.keys(bindings).forEach((name) => {
combinedBindings[name.toUpperCase()] = bindings[name]
? bindings[name]
: '';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
combinedBindings[name.toUpperCase()] = bindings[name]
? bindings[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
* @param {?string} message
*/
function throwError_(message) {
throw new Error(message);
}

/**
* @private
* @param {?string} message
*/
function throwWarning_(message) {
console.warn(message);
}

/**
* @private
*/
function handleActivation_() {
if (finalEndpoint.split(':', 1)[0] === 'navigator-share') {
if (window && window.navigator && window.navigator.share) {
const dataStr = finalEndpoint.substr(finalEndpoint.indexOf('?'));
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a util for this in the url.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

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 that's parseUrl().queryString.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 # in this URL, what if there's no ?, etc.

Copy link
Contributor Author

@krdwan krdwan May 13, 2020

Choose a reason for hiding this comment

The 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.

  1. No ? -> returns '' (empty string)
  2. ? and # -> only returns the substring from ? to #
  3. # then ? (reversed order) -> returns substring from ? to the end of string
  4. ? and no #-> returns substring from ? to the end of string

const data = parseQueryString(dataStr);
window.navigator.share(data).catch((e) => {
throwWarning_(`${e.message}. ${name}`);
});
} else {
throwWarning_(
`Could not complete system share. Navigator unavailable. ${name}`
);
}
} 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;
const baseStyle = CSS.BASE_STYLE;
const backgroundStyle = CSS[type];
//Default to gray (d3d3d3) background if type is not preconfigured
const backgroundStyle = CSS[type.toUpperCase()] || {
'backgroundColor': 'd3d3d3',
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have # in front?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll consolidate into CSS file to make it easier to understand. The current formatting does not require #.

Copy link
Contributor

Choose a reason for hiding this comment

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

These are not CSS values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is a CSS value, but this is the format that the component consumes it in, (does not require the #.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. I don't like it. But I guess let's run with what they accept and we'll see if that causes problems later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!

};
const size = {
width: props['width'] || DEFAULT_WIDTH,
height: props['height'] || DEFAULT_HEIGHT,
width: checkedWidth,
Copy link
Member

Choose a reason for hiding this comment

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

Let's floor these instead of checking if they're integers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

height: checkedHeight,
};
useResourcesNotify();
return (
<div
role="button"
tabindex={props['tabIndex'] || '0'}
onKeyDown={handleKeyPress}
onClick={handleActivation}
onKeyDown={handleKeyPress_}
onClick={handleActivation_}
style={{...size, ...props['style']}}
{...props}
>
<SocialShareIcon
style={{...backgroundStyle, ...baseStyle, ...size}}
type={type}
type={type.toUpperCase()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's get the type.toUpperCase() value once somewhere up top and use it elsewhere.

/>
</div>
);
Expand Down
85 changes: 85 additions & 0 deletions extensions/amp-social-share/0.2/storybook/Basic-Social-Share2.js
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 {SocialShare2} 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 2',
Copy link
Contributor

Choose a reason for hiding this comment

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

0.2

component: SocialShare2,
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>
<SocialShare2
type={type}
href={href}
shareEndpoint={href}
params={params}
bindings={bindings}
width={width}
height={height}
/>
</div>
);
};