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 8 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
93 changes: 0 additions & 93 deletions extensions/amp-social-share/0.2/social-share.js

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,6 @@ const BUILTINS = dict({
'mini': 'true',
},
},
'gplus': {
'obsolete': true,
},
'email': {
'bindings': ['recipient'],
'shareEndpoint': 'mailto:RECIPIENT',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,6 @@ export const SMS = {'backgroundColor': 'ca2b63'};

/* "system" styling */
export const SYSTEM = {'backgroundColor': '000000'};

/* "default" styling */
export const DEFAULT = {'backgroundColor': 'd3d3d3'};
210 changes: 210 additions & 0 deletions extensions/amp-social-share/1.0/social-share.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,210 @@
/**
* 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 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 {useResourcesNotify} from '../../../src/preact/utils';

const DEFAULT_WIDTH = 60;
const DEFAULT_HEIGHT = 44;

/**
* @param {!JsonObject} props
* @return {PreactDef.Renderable}
*/
export function SocialShare(props) {
const name = 'SocialShare';
Copy link
Contributor

Choose a reason for hiding this comment

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

Make a top-level const.

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!

useResourcesNotify();

const {
'typeConfig': typeConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

The checkProps should return a struct type and no quotes should be used here. E.g.

const {baseEndpoint, ...} = checkProps(props);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will make the change. Per your previous comment: #28137 (comment)

Is the correct convention to use quotes when unpacking the props only? And then for unpacking any other object do not use quotes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. For now this is the convention. Any internal structure should have a type declaration that could be a struct. Then we don't need quotes. Unfortunately we can't control a value that's passed to us from outside, such as component props. Another way to think about it is obfuscation. I.e. do we want an obfuscation for any structure? We can't obfuscate component props or component output. For instance, one of such props could be disabled. If we obfuscate it to d and output it to JSX - a DOM element expects disabled. But the structures you use internally are ok to obfuscate. E.g. typeConfig will become tc or such. But it's not a visible value to anyone but our internal code.

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. Thanks for clarifying how the obfuscater interacts with properties passed from outside the component. I have removed the quotes here, and included quotes inside of checkProps where the props are unpacked.

'baseEndpoint': baseEndpoint,
'checkedWidth': checkedWidth,
'checkedHeight': checkedHeight,
} = checkProps(props, name);
const finalEndpoint = createEndpoint(typeConfig, baseEndpoint, props);

/**
* @private
* @param {!JsonObject} typeConfig
* @param {?string} baseEndpoint
* @param {!JsonObject} props
* @return {?string}
*/
function createEndpoint(typeConfig, baseEndpoint, props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to a top-level function. It's a noticeable cost to have these inlined w/o good reason.

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, sounds good. I'll move all static functions outside of Component going forward.

const {params, bindings} = props;
const combinedParams = dict();
Object.assign(combinedParams, typeConfig['defaultParams'], params);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use const combinedParams = ({...typeConfig['defaultParams'], params}).

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!

const endpointWithParams = addParamsToUrl(baseEndpoint, combinedParams);

const combinedBindings = dict();
const bindingVars = typeConfig['bindings'];
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]),
endpointWithParams
);
return finalEndpoint;
}

/**
* @private
*/
function handleActivation() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto: move to top level and just pass finalEndpoint as an arg.

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!

const protocol = finalEndpoint.split(':', 1)[0];
const windowFeatures = 'resizable,scrollbars,width=640,height=480';
if (protocol === 'navigator-share') {
if (window && window.navigator && window.navigator.share) {
const dataStr = finalEndpoint.substr(finalEndpoint.indexOf('?'));
dvoytenko marked this conversation as resolved.
Show resolved Hide resolved
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 if (protocol === 'sms') {
openWindowDialog(
window,
finalEndpoint.replace('?', '?&'),
'_blank',
windowFeatures
);
} else {
openWindowDialog(window, finalEndpoint, '_blank', windowFeatures);
}
}

/**
* @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] || CSS.DEFAULT;
const size = {
width: checkedWidth,
height: checkedHeight,
};
return (
<div
role="button"
tabindex={props['tabIndex'] || '0'}
onKeyDown={handleKeyPress}
onClick={handleActivation}
style={{...size, ...props['style']}}
{...props}
>
<SocialShareIcon
style={{...backgroundStyle, ...baseStyle, ...size}}
type={type}
/>
</div>
);
}

/**
* @param {!JsonObject} props
* @param {?string} name
* @return {!JsonObject}
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 return a struct type.

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!

*/
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.

We can remove name from args and just use NAME everywhere instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, updated!

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
// Silently assigns default for undefined, null
// Throws Warning for booleans, strings, 0, negative numbers
// No errors when positive integer or equivalent string
let checkedWidth =
width === null || width === undefined ? DEFAULT_WIDTH : width;
Copy link
Contributor

Choose a reason for hiding this comment

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

x === null || x === undefined is x == null. But also we should be able to use ?? now. E.g. this expression can be done as width ?? DEFAULT_WIDTH.

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 to ?? and removed the if statement underneath.

let checkedHeight =
height === null || height === undefined ? DEFAULT_HEIGHT : height;
if (typeof checkedWidth === 'boolean' || !(Math.floor(checkedWidth) > 0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this if, tbh. Why would we expect the width to be a boolean vs, say, a function or an object? What would math.floor() do if the width was indeed a string? IMHO, this whole warning we can do without.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, the reason I have this check for boolean is because Math.floor(true) actually returns 1, which would then not trigger the warning. Math.floor on a string converts the string to a number if it is a valid number or returns NaN which would trigger the warning.

So in combination with the above, null and undefined would silently be reassigned to be DEFAULT. Strings, objects, booleans, functions would trigger the warning. Valid positive strings and numbers will pass through unchanged and not trigger the warning.


The reasoning for including this was to be thorough so that in the case where the user accidentally passes an invalid width (or height), the component would resize to default and the user may not understand why.

Sounds like you think this is too much detail to check for. I am fine with removing the whole thing if you think so.

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, unless the logic differs by type, we shouldn't check types. Eventually we'll have to look into PropTypes or d.ts to define the prop types. Additionally, fractional values are fine as well, so we don't need to do Math.floor().

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, I have updated this. There is no type checking now. It will only do the null coalescing operation (??). We can update the type checking in the future when we get PropTypes

throwWarning(
`The width property should be a positive integer of type Integer or String, defaulting to ${DEFAULT_WIDTH}. ${name}`
);
checkedWidth = DEFAULT_WIDTH;
}
if (typeof checkedHeight === 'boolean' || !(Math.floor(checkedHeight) > 0)) {
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);
}
73 changes: 73 additions & 0 deletions extensions/amp-social-share/1.0/storybook/Basic.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/**
* 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 {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('type', knobConfigurations, knobConfigurations[0]);
const href = text('shareEndpoint', 'Not Specified');
const params = object('params', {'subject': 'test'});
const bindings = object('bindings', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, ok to defer to a follow up PR, but this prop bugs me the most. Putting on a user's hat on, I really don't understand why there are two params and bindings, and what's the difference between them.

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, I have created a comment in the parent issue to track outstanding items.

#28283 (comment)

'canonical_url': 'test2',
'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>
);
};