📖 [amp-social-share][storybook][preact] Add basic bento storybook story for amp-social-share component #27745
Conversation
|
@caroqliu Would you mind giving this a quick review to make sure I am using social-share correctly in the context of bento? Thanks in advance! |
There was a problem hiding this comment.
This looks good so far in terms of elaborating all of the pre-configurable providers for amp-social-share. Could you also include a knob for specifying an arbitrary href prop and logo for non-configured-providers?
cc @wassgha Is it possible yet to pass in children props? This could be helpful for the non-configured case above when publishers want to pass in custom logos to social share.
|
Yup! @krdwan do you need help making that example? |
Thanks for the feedback @caroqliu. I'll look into the non-configured-providers and follow up on the next commit. |
@caroqliu Please see latest changes, I added a knob to specify a custom href prop. From my understanding, the preact version of the component cannot customize the logo via props (it has to rely on the type prop which picks from the pre-defined logos). Can you let me know if this is incorrect? |
There was a problem hiding this comment.
Thanks for the modifications, Kevin. Could we also add a storybook sample for AMP mode in addition to Preact mode? This can help us directly compare how significant the differences in usage is between the modes, and more tangibly how to close this gap smoothly.
| 'line', | ||
| 'sms', | ||
| 'system', | ||
| 'custom endpoint', |
There was a problem hiding this comment.
Is it possible to create an input for type that's outside of the preconfigured providers? i.e. a select dropdown of preconfigured providers or an empty input to configure the type prop value, since really publishers can put anything they want, not just "custom endpoint".
There was a problem hiding this comment.
It is possible, but any 'type' attribute that is not one of the pre-configured types, actually does not change the behavior of the component. So having the 'custom endpoint' as a catch all should address this use case. Unless there is something else I'm missing.
There was a problem hiding this comment.
@caroqliu could you clarify? It also looks to me that a single "custom endpoint" should suffice. Do you see the need for more?
There was a problem hiding this comment.
Feel free to consider my suggestion as optional. I am asking for the freeform input knob for the sake of completeness of the possible publisher-facing experience. It's true that technically the "custom endpoint" option covers that codepath.
|
Thanks for feedback @dvoytenko @caroqliu . Based on discussions from last week and comments above, it sounds like we want to re-design the preact version of this component. Since this PR is dependent on the final behavior of the re-designed component, I will hold off on making any additional changes here. To help with comparing the behavior, I have created a PR to add stories for the amp version of the component, which can be found here: #27906 For reference, the current state behavior is such --
|
|
@krdwan It sounds like we want to change how Preact component works. And the good time to do it is essentially now. Let's complete this story book as a highlight what we want to change and then start addressing this issues in the follow up pull requests. But I agree with @caroqliu - let's add AMP storybook as well right away. I'd really like to see them side-by-side - that will help us design the right APIs for both. |
@dvoytenko Sounds good, I am onboard with merging this story and the amp version story so we can compare side by side. Once everyone has a chance to play around with it in storybook, I am happy to make further updates. I am currently now allowed to merge so if someone wants to merge for me please do :) Also the amp version story is in review, if anyone wants to review / approve it please do :) |
| @@ -83,6 +83,7 @@ export function SocialShare(props) { | |||
| onClick={handleActivation} | |||
| style={{...size, ...props['style']}} | |||
| {...props} | |||
| aria-label={`amp-social-share component of type: ${type}`} | |||
There was a problem hiding this comment.
We should probably make this a bit more user-friendly along the lines of 'Share on ${type}' ?
There was a problem hiding this comment.
Yeah, certainly. It's a user-readable string.
…hare component (ampproject#27745) * Basic bento story for amp-social-share component * Update amp-social-share story to allow custom endpoint
krdwan
changed the title
krdwan
changed the title
Add basic storybook story for amp-social-share component
Includes a change to preact amp-social-share file to include aria-label attribute. (This is used for accessibility, and without the label an accessibility error is triggered in storybook.)
aria-label is used for screenreaders
My understanding of bento is limited so please feel free to recommend changes