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

krdwan · 2020-04-30T16:04:47Z

Partial for #28283

Moved logic to configure the share endpoint with search params into preact component. This does not include changes needed to amp component.

Open questions

Number.isInteger() causing an Es-lint issue. What is the preferred way of doing this?
Proper Usage of UseResourcesNotify() confirm?
Proper Usage of window.navigator.share (will this cause a problem in server side render?). From y research window appears available to react components.

4/30 --
Latest commit updates the Number.isInteger to use Math.floor()
Adds in a storybook for this component

5/11 --
Renamed from 0.2 to 1.0 for directory name

alanorozco · 2020-04-30T20:08:38Z

extensions/amp-social-share/0.2/social-share.js

  const size = {
-    width: props['width'] || DEFAULT_WIDTH,
-    height: props['height'] || DEFAULT_HEIGHT,
+    width: checkedWidth,


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

alanorozco · 2020-04-30T20:09:16Z

extensions/amp-social-share/0.2/social-share.js

+        combinedBindings[name.toUpperCase()] = bindings[name]
+          ? bindings[name]
+          : '';


Suggested change

combinedBindings[name.toUpperCase()] = bindings[name]

? bindings[name]

: '';

combinedBindings[name.toUpperCase()] = bindings[name] ?? '';

alanorozco · 2020-04-30T20:09:42Z

extensions/amp-social-share/0.2/social-share.js

+    const bindingVars = typeConfig['bindings'];
+    if (bindingVars) {
+      bindingVars.forEach((name) => {
+        combinedBindings[name.toUpperCase()] = params[name] ? params[name] : '';


Suggested change

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

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

alanorozco · 2020-04-30T20:10:52Z

extensions/amp-social-share/0.2/social-share.js

+    const combinedParams = dict();
+    Object.assign(combinedParams, typeConfig['defaultParams'], params);


Does the linter complain using this form?

Suggested change

const combinedParams = dict();

Object.assign(combinedParams, typeConfig['defaultParams'], params);

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

alanorozco · 2020-04-30T20:13:24Z

extensions/amp-social-share/0.2/social-share.js

+    }
+
+    // Throw warning if type is obsolete
+    const obsoleteType = typeConfig['obsolete'];


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.

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.

Yeah, let's remove :)

dvoytenko · 2020-04-30T21:18:05Z

extensions/amp-social-share/0.2/social-share.js

@@ -30,63 +32,196 @@ const DEFAULT_HEIGHT = 44;
 * @param {!JsonObject} props
 * @return {PreactDef.Renderable}
 */
-export function SocialShare(props) {
+export function SocialShare2(props) {


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?

Yes. Let's break.

dvoytenko · 2020-04-30T21:19:24Z

extensions/amp-social-share/0.2/social-share.js

-      event.preventDefault();
-      handleActivation();
+  function checkProps_(props, name) {
+    const {type, shareEndpoint, params, bindings, width, height} = props;


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

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

dvoytenko · 2020-04-30T21:21:29Z

extensions/amp-social-share/0.2/social-share.js

-    if (key == Keys.SPACE || key == Keys.ENTER) {
-      event.preventDefault();
-      handleActivation();
+  function checkProps_(props, name) {


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

dvoytenko · 2020-04-30T21:23:10Z

extensions/amp-social-share/0.2/social-share.js

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


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.

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.

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.

Makes sense, let's throw the error explicitly.

dvoytenko · 2020-04-30T21:24:51Z

extensions/amp-social-share/0.2/social-share.js

+      throwError_(`The type attribute is required. ${name}`);
+    }
+    if (/\s/.test(type)) {
+      throwError_(


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

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.

Let's remove.

extensions/amp-social-share/0.2/social-share.js

dvoytenko · 2020-04-30T21:34:42Z

extensions/amp-social-share/0.2/social-share.js

+  function handleActivation_() {
+    if (finalEndpoint.split(':', 1)[0] === 'navigator-share') {
+      if (window && window.navigator && window.navigator.share) {
+        const dataStr = finalEndpoint.substr(finalEndpoint.indexOf('?'));


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

Looks like there is getFragment() which gets params after '#' symbol but nothing for '?' unless I'm missing somehting.

I think that's parseUrl().queryString.

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.

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.

No ? -> returns '' (empty string)

? and # -> only returns the substring from ? to #

# then ? (reversed order) -> returns substring from ? to the end of string

? and no #-> returns substring from ? to the end of string

dvoytenko · 2020-04-30T21:35:50Z

extensions/amp-social-share/0.2/social-share.js

-  const backgroundStyle = CSS[type];
+  //Default to gray (d3d3d3) background if type is not preconfigured
+  const backgroundStyle = CSS[type.toUpperCase()] || {
+    'backgroundColor': 'd3d3d3',


This should have # in front?

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

These are not CSS values?

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

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.

Sounds good!

dvoytenko · 2020-04-30T21:36:50Z

extensions/amp-social-share/0.2/social-share.js

      style={{...size, ...props['style']}}
      {...props}
    >
      <SocialShareIcon
        style={{...backgroundStyle, ...baseStyle, ...size}}
-        type={type}
+        type={type.toUpperCase()}


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

dvoytenko · 2020-04-30T21:37:40Z

extensions/amp-social-share/0.2/storybook/Basic-Social-Share2.js

+import {withA11y} from '@storybook/addon-a11y';
+
+export default {
+  title: 'Social Share 2',


krdwan · 2020-05-01T19:30:09Z

Thanks for review @alanorozco @dvoytenko . Next revision is up!

caroqliu

Looking forward to see how this impacts AMP mode!

caroqliu · 2020-05-01T20:24:08Z

extensions/amp-social-share/0.2/social-share.css.js

@@ -54,3 +54,6 @@ export const SMS = {'backgroundColor': 'ca2b63'};

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


Why is this needed?

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.

caroqliu · 2020-05-01T20:32:24Z

extensions/amp-social-share/0.2/social-share.js

+    const endpointWithParams = addParamsToUrl(baseEndpoint, combinedParams);
+
+    const combinedBindings = dict();
+    const bindingVars = typeConfig['bindings'];


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?

caroqliu · 2020-05-01T20:37:37Z

extensions/amp-social-share/0.2/social-share.js

+  }
+
+  // Verify width and height are valid integers
+  let checkedWidth = Math.floor(width || DEFAULT_WIDTH);


This is an interesting case: Math.floor(null) yields 0 but Math.floor(undefined) yields NaN -- if the prop isn't present it's undefined, meaning it would be reasonable to rewrite this: Math.floor(width) || DEFAULT_WIDTH

Do we ever expect width to be null? That would be a reasonable case for the current order.

Edit: Just saw the positive integer check below, in which case even if it is null we would prefer to use the DEFAULT_WIDTH.

tldr supernit: Math.floor(width) || DEFAULT_WIDTH

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
// Silently assigns default for undefined, null
// Throws Warning for booleans, strings, 0, negative numbers
// No errors when positive integer or equivalent string

caroqliu · 2020-05-01T20:38:10Z

extensions/amp-social-share/0.2/social-share.js

+  // Verify width and height are valid integers
+  let checkedWidth = Math.floor(width || DEFAULT_WIDTH);
+  let checkedHeight = Math.floor(height || DEFAULT_HEIGHT);
+  if (!(checkedWidth > 0)) {


nit: checkedWidth <= 0

Thanks, I've re-written this seciton, although kept this part with > 0 because when Math.floor returns NaN, it always return false in a comparison so the recommended update would return a different behavior.

caroqliu · 2020-05-01T20:39:20Z

extensions/amp-social-share/0.2/social-share.js

+    );
+    checkedWidth = DEFAULT_WIDTH;
+  }
+  if (!(checkedHeight > 0)) {


checkedHeight <= 0

krdwan · 2020-05-11T20:12:31Z

Thanks again for the review @caroqliu @dvoytenko @alanorozco

I've made all the changes besides the ones for params / bindings. My proposal for next steps is the following

Merge the current version of preact component
Work on amp component updates
Understand with regard to both preact and amp what makes the most sense for us to do
Create a separate PR that will include amp component and changes to preact if needed (which may include a change to the config)

Let me know what you guys think!

Also I moved everything into a directory called 1.0, but this makes it hard to diff. Let me know if you want me to move it back to 0.2 and then make the 1.0 change in a separate PR.

Here's my latest thoughts on the params / bindings discussion. #28137 (comment)

dvoytenko · 2020-05-12T15:55:32Z

@krdwan merging this soon and iterating is a good idea. However, please make sure the remaining tasks are listed in #28283.

dvoytenko

A few pre-LGTM comments.

dvoytenko · 2020-05-12T15:56:49Z

extensions/amp-social-share/1.0/social-share.js

+/**
+ * @param {!JsonObject} props
+ * @param {?string} name
+ * @return {!JsonObject}


This should return a struct type.

dvoytenko · 2020-05-12T15:57:52Z

extensions/amp-social-share/1.0/social-share.js

+  useResourcesNotify();
+
+  const {
+    'typeConfig': typeConfig,


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

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

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?

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.

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.

dvoytenko · 2020-05-12T15:58:10Z

extensions/amp-social-share/1.0/social-share.js

+ * @return {PreactDef.Renderable}
+ */
+export function SocialShare(props) {
+  const name = 'SocialShare';


Make a top-level const.

dvoytenko · 2020-05-12T15:59:10Z

extensions/amp-social-share/1.0/social-share.js

+   * @param {!JsonObject} props
+   * @return {?string}
+   */
+  function createEndpoint(typeConfig, baseEndpoint, props) {


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

Updated, sounds good. I'll move all static functions outside of Component going forward.

dvoytenko · 2020-05-12T16:00:32Z

extensions/amp-social-share/1.0/social-share.js

+  function createEndpoint(typeConfig, baseEndpoint, props) {
+    const {params, bindings} = props;
+    const combinedParams = dict();
+    Object.assign(combinedParams, typeConfig['defaultParams'], params);


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

dvoytenko · 2020-05-12T16:02:35Z

extensions/amp-social-share/1.0/social-share.js

+  /**
+   * @private
+   */
+  function handleActivation() {


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

extensions/amp-social-share/1.0/social-share.js

dvoytenko · 2020-05-12T16:07:01Z

extensions/amp-social-share/1.0/social-share.js

+  // 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;


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.

Updated to ?? and removed the if statement underneath.

dvoytenko · 2020-05-12T16:11:01Z

extensions/amp-social-share/1.0/social-share.js

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


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.

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.

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().

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

dvoytenko · 2020-05-12T16:13:59Z

extensions/amp-social-share/1.0/storybook/Basic.js

+  const type = select('type', knobConfigurations, knobConfigurations[0]);
+  const href = text('shareEndpoint', 'Not Specified');
+  const params = object('params', {'subject': 'test'});
+  const bindings = object('bindings', {


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.

Sounds good, I have created a comment in the parent issue to track outstanding items.

#28283 (comment)

krdwan · 2020-05-13T20:42:53Z

@krdwan merging this soon and iterating is a good idea. However, please make sure the remaining tasks are listed in #28283.

Thanks for the very thorough review @dvoytenko . I have noted that we will re-visit the way params and bindings are structured in this component as we design out the higher level amp component.

#28283 (comment)

jridgewell · 2020-05-13T21:14:04Z

extensions/amp-social-share/1.0/social-share.js

+  if (q < 0) {
+    return '';
+  } else if (h < q) {
+    return endpoint.substr(q);


This doesn't account for foo.html#this?is-a-hash-string. Query must come before a hash. We could normalize if (h === -1) { h = endpoint.length } to solve this.

Thanks Justin, I've updated this function with your recommendation. Are there any other use cases you can think of? Or a util that I'm not aware of that already does this?

I think I mentioned it elsewhere, but url.js's parseUrl().queryString should be able to do this.

Good point on parseUrlDeprecated deprecation. @jridgewell do you know what we deprecated it with? Is there something else we should be using now?

jridgewell · 2020-05-13T21:20:05Z

extensions/amp-social-share/1.0/social-share.js

+  }
+  if (bindings) {
+    Object.keys(bindings).forEach((name) => {
+      combinedBindings[name.toUpperCase()] = bindings[name] || '';


I don't think we intended for any binding to be used here, right?

Could you elaborate on your question? In this section, the bindings object coming from props is being added into the combinedBindings. Then in the next part we replace variables in finalEndpoint with bindings from combinedBindings.

I can pass in bindings: { foobar: 'x' }, where foobar is some random binding name, and not one of the typeConfig['bindings']. I think this is part of the confusion with having params and binding, we're allowing generic find-and-replace to happen where we meant to just allow you to replace a few strings in our static config JSON with something else.

I see your concern. The API from the original component behaves as you described.

In the previous version of the component, the binding find and replace happens asynchronously (in urlReplacements.expandUrlAsync()) and was limited to only those bindings included in our config.

Now the logic is split into two parts, the (synchronous) find and replace logic lives inside the preact component and we will would have to do an asynchronous call within the amp component (since this needs to be within the amp environment) to get our binding values. Then pass the bindings into our preact component.

As a side effect to this, someone using the preact component in isolation would be able to add additional generic bindings like you described (bindings: {foobar: 'x' }. I don't see this as a problem though since the user actually gets more flexibility with how they want to use the component and could potentially add other variables specific to the preact environment.

I think the model we should try to get to is that the component only has a notion of props/params. And the whole async binding->params mapping is done wholly on the AMP side.

Sounds good, let's work toward this model in the AMP component design!

…# params

jridgewell · 2020-05-13T22:19:37Z

extensions/amp-social-share/1.0/social-share.js

@@ -194,7 +194,7 @@ function getQueryString(endpoint) {
  let h = endpoint.indexOf('#');
  q = q === -1 ? endpoint.length : q;
  h = h === -1 ? endpoint.length : h;
-  return h < q ? '' : endpoint.substr(q, h - q);
+  return endpoint.substr(q, h - q);


Nit: We can use slice since it uses endIndex param instead of length:

Suggested change

return endpoint.substr(q, h - q);

return endpoint.slice(q, h);

Thanks, this is much cleaner, updated!

jridgewell · 2020-05-13T22:24:05Z

extensions/amp-social-share/1.0/social-share.js

+  }
+  if (bindings) {
+    Object.keys(bindings).forEach((name) => {
+      combinedBindings[name.toUpperCase()] = bindings[name] || '';


I can pass in bindings: { foobar: 'x' }, where foobar is some random binding name, and not one of the typeConfig['bindings']. I think this is part of the confusion with having params and binding, we're allowing generic find-and-replace to happen where we meant to just allow you to replace a few strings in our static config JSON with something else.

dvoytenko

LGTM with two comments. The next step here should definitely be unwinding the confusion between bindings and params. It's certainly ok to do a detour to AMP part first, but we'll likely bump into the same issue there right away.

dvoytenko · 2020-05-14T00:30:57Z

extensions/amp-social-share/1.0/social-share.js

+/**
+ * @param {!JsonObject} props
+ * @param {?string} name
+ * @return {struct}


This will look like this:

@return {{ typeConfig: !JsonObject, baseEndpoint: string, checkedWidth: number, checkedHeight: number, }}

Thanks, I have updated the struct definition!

dvoytenko · 2020-05-14T00:31:31Z

extensions/amp-social-share/1.0/social-share.js

+ *   {number} checkedWidth
+ *   {number} checkedHeight
+ */
+function checkProps(props, name) {


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

Thank you, updated!

First draft, new version of preact social-share component

09c3686

googlebot added the cla: yes label Apr 30, 2020

krdwan added 3 commits April 30, 2020 15:43

Moved contents to social-share and added story

2baa59a

delete old version of social-share2

4474689

Adding missed change to story

a87faf1

alanorozco reviewed Apr 30, 2020

View reviewed changes

dvoytenko suggested changes Apr 30, 2020

View reviewed changes

krdwan added 2 commits May 1, 2020 15:23

Revisions from initial code review

3bec731

renamed private funcitons in amp-social-share

8c308e1

caroqliu reviewed May 1, 2020

View reviewed changes

caroqliu mentioned this pull request May 8, 2020

[amp-social-share:1.0] Bento tracking issue #28283

Closed

25 tasks

krdwan added 2 commits May 11, 2020 11:24

Updated storybook and social-share, renamed from 0.2 to 1.0

32e0158

Adding missed change on amp social share

000dfb1

krdwan marked this pull request as ready for review May 12, 2020 14:51

dvoytenko suggested changes May 12, 2020

View reviewed changes

krdwan added 5 commits May 13, 2020 15:54

More changes to preact component

c5ed252

Add some minor changes in how props are unpacked

9caade3

Removed a typo

5eb95c8

update getQueryString helper

e58dfb0

Fix a typo

4e57360

jridgewell reviewed May 13, 2020

View reviewed changes

krdwan added 4 commits May 13, 2020 17:25

Merge with master

4a2054d

Update to getQueryString helper for use case where a ? exists within …

96b90b2

…# params

Cleaner syntax in getQueryString helper

e6609df

Even cleaner more elegant syntax for getQueryString

e3d162b

jridgewell reviewed May 13, 2020

View reviewed changes

Update getQueryString, update versioning from 0.2 to 1.0

b778405

krdwan changed the title ~~✨ First draft, new version of preact social-share component~~ ✨ New version of preact social-share component May 14, 2020

dvoytenko approved these changes May 14, 2020

View reviewed changes

krdwan added 7 commits May 14, 2020 10:19

Fixed linter issue, update struct definition

fb01ee0

console /*OK*/.warn to pass linter error (warn in preact env)

90de0eb

update config for 0.2 -> 1.0

bd2a7b5

linter updates

03e4c56

Adding parenthesis around type casts

19dd93a

Updates for type checking

5ad27c2

removed null coalescing operator to pass pre-merge checks

ac3c294

This was referenced May 14, 2020

🚨 Error: gulp dist --fortesting --single_pass failing for ?? operator within preact component (bento) #28429

Closed

gulp dist --fortesting --single_pass failing for ?? operator within preact component (bento) #28432

Closed

alanorozco merged commit 62de49f into ampproject:master May 14, 2020

krdwan changed the title ~~✨ New version of preact social-share component~~ ✨[bento][amp-social-share] New version of preact social-share component May 14, 2020

krdwan changed the title ~~✨[bento][amp-social-share] New version of preact social-share component~~ ✨[bento][amp-social-share][preact] New version of preact social-share component May 14, 2020

krdwan changed the title ~~✨[bento][amp-social-share][preact] New version of preact social-share component~~ ✨[amp-social-share][bento][preact] New version of preact social-share component May 14, 2020

ampprojectbot added the PR use: In Beta / Experimental label May 19, 2020

ampprojectbot added PR use: In Stable and removed PR use: In Stable labels May 27, 2020

krdwan mentioned this pull request Jun 16, 2020

✨ [amp-social-share][bento][amp] Amp-social-share for bento with some re-design of preact component #28465

Merged

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

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

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

	return endpoint.substr(q, h - q);
	return endpoint.slice(q, h);

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

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

Conversation

krdwan commented Apr 30, 2020 • edited by dvoytenko

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krdwan May 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krdwan commented May 1, 2020

caroqliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krdwan commented May 11, 2020 • edited

dvoytenko commented May 12, 2020

dvoytenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krdwan commented May 13, 2020

krdwan commented Apr 30, 2020 •

edited by dvoytenko

krdwan May 13, 2020 •

edited

krdwan commented May 11, 2020 •

edited