search

Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.42k stars 1.99k forks source link

(3P) Social Previews: Research for WPCOM reader preview inclusion #44802

Open obenland opened 4 years ago

obenland commented 4 years ago

https://github.com/Automattic/wp-calypso/issues/44380

cpapazoglou commented 4 years ago

What this issue will ultimately resolve

We need the WordPress.com Reader Preview to be available in Jetpack (both standalone and wpcom mode) as we already have facebook, twitter, google search.

Problem

Constants

Options

  1. Recreate the component in @automattic/social-previews. Given the Constants above we don't need anything from the state

    • The most straightforward way as of now.
    • Can be used anywhere without any wpcom dependencies. It will just mimic the appearance of the reader as we already do for facebook, twitter, google search
    • Very bad maintainability, we will have to maintain two copies of the component and also publish the changes to npm

  2. Create a wrapper component in calypso that will provide any state needed. We could then try to make the inner components more abstract and create npm packages for them. Ultimately we would replace all the inner references with the new npm packages and reuse them in jetpack

    • DRY code
    • Not 100% sure it will work that way
    • It touches a lot of components / blocks that are also used by other components, so the complexity goes 🚀
    • Feature minor changes to the components being npm packages will require more work

I am trying hard to find a better solution than the two above, which are both far from perfect. If we drop the standalone jetpack environment, maybe it is worth trying to find a way sharing the component from calypso and making it usable in jetpack https://github.com/Automattic/jetpack/blob/a77a757b2629cd0a9cb4b95e45a15c79f2b4ac4f/extensions/blocks/social-previews/constants.js#L5-L28

getdave commented 4 years ago

44380

Simple but effective 🥇

getdave commented 4 years ago

@cpapazoglou Thanks for researching and providing options. Personally I vote for option 1 - Recreate the component in @automattic/social-previews..

Very bad maintainability,

Can you provide info on how many times the Calypso reader component(s) have been directly modified in the last year? That will give us some idea of the impact and which teams/individuals we should consult before going ahead.

Can be used anywhere without any wpcom dependencies

I'd say that's a pretty major plus point.

The most straightforward way as of now.

If we want this included quickly to provide feature parity between the Calypso Preview modal and the new Social Previews in the Jetpack sidebar then we should take this route.

cpapazoglou commented 4 years ago

Can you provide info on how many times the Calypso reader component(s) have been directly modified in the last year? That will give us some idea of the impact and which teams/individuals we should consult before going ahead.

As it seems here https://github.com/Automattic/wp-calypso/commits/master/client/blocks/reader-post-card there are ~ 10 commits the last year which actually touched the appearance (since we don't care for functionality) especially typography. The good thing is that almost all of them are CSS related which should be easy enough to port in the npm package.

I suggest:

How does that sound @getdave ?

cpapazoglou commented 4 years ago

There is WIP in this PR https://github.com/Automattic/wp-calypso/pull/45056

github-actions[bot] commented 3 years ago

This issue is stale because it has been 180 days with no activity. You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation and apply one of relevant issue close labels.