Skip to content

Conversation

@marekhrabe
Copy link
Contributor

@marekhrabe marekhrabe commented Aug 4, 2020

Fixes #16196
Fixes #16729
Fixes #16785
Fixes #16805

Changes proposed in this Pull Request:

  • Adds an actual modal containing social previews that are connected to the current post - author, url, post title, excerpt/content and the featured image…
  • Working controls for switching between services
  • This PR only implements both the functionality and styling based on the design from the issue

Screenshots

Twitter Facebook Google Upgrade
Screenshot 2020-08-06 at 18 02 23 Screenshot 2020-08-06 at 18 02 16 Screenshot 2020-08-06 at 18 02 07

Jetpack product discussion

Does this pull request change what data or activity we track or use?

no

Testing instructions:

Proposed changelog entry for your changes:

none yet

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello marekhrabe! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D47531-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@marekhrabe marekhrabe force-pushed the add/social-preview-modal branch from a6cb9e0 to 27989e0 Compare August 4, 2020 14:20
Base automatically changed from add/social-preview-sidebar to master August 5, 2020 11:07
@marekhrabe marekhrabe force-pushed the add/social-preview-modal branch from 27989e0 to d6117f5 Compare August 5, 2020 11:34
@jetpackbot
Copy link
Collaborator

jetpackbot commented Aug 5, 2020

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16704

Generated by 🚫 dangerJS against 073cd0c

@marekhrabe marekhrabe force-pushed the add/social-preview-modal branch from aba0223 to 1b86146 Compare August 5, 2020 14:45
@marekhrabe marekhrabe marked this pull request as ready for review August 5, 2020 15:18
@marekhrabe marekhrabe added [Status] Needs Review This PR is ready for review. [Status] Needs Team Review Obsolete. Use Needs Review instead. labels Aug 5, 2020
@marekhrabe
Copy link
Contributor Author

Design followup #16730

@retrofox
Copy link
Contributor

retrofox commented Aug 5, 2020

Tested in the functionality works as expected.

image

jeherve
jeherve previously approved these changes Aug 5, 2020
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

It works nicely for me. 👍

@jeherve jeherve removed the [Status] Needs Review This PR is ready for review. label Aug 5, 2020
retrofox
retrofox previously approved these changes Aug 5, 2020
Copy link
Contributor

@retrofox retrofox left a comment

Choose a reason for hiding this comment

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

It works as expected. Left some suggestions. LGTM.

@marekhrabe marekhrabe dismissed stale reviews from retrofox and jeherve via 5f0bb44 August 6, 2020 08:39
@marekhrabe marekhrabe force-pushed the add/social-preview-modal branch from a430de1 to d698c3b Compare August 6, 2020 09:41
@marekhrabe
Copy link
Contributor Author

Everything blocking has been addressed and this should be ready to get one more review!

@marekhrabe marekhrabe requested review from a team August 6, 2020 10:02
@marekhrabe marekhrabe added the [Status] Needs Review This PR is ready for review. label Aug 6, 2020
@marekhrabe marekhrabe force-pushed the add/social-preview-modal branch from 551b04a to 0f70853 Compare August 11, 2020 13:43
@marekhrabe
Copy link
Contributor Author

Rebased and updated D47531-code for dotcom testing.

getdave
getdave previously approved these changes Aug 11, 2020
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Another pass at this following the updates and rebase. Tested by forcing available and unavailable cases and both instances look good.

@marekhrabe marekhrabe removed the [Status] Needs Team Review Obsolete. Use Needs Review instead. label Aug 11, 2020
Copy link
Contributor

@frontdevde frontdevde left a comment

Choose a reason for hiding this comment

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

Addressed @jeryj's a11y improvement suggestion via a minor commit and retested to ensure it's still working as expected.

LGTM 👍

Comment on lines +126 to +127
grid-gap: 3em;
grid-template-columns: 1fr 1fr;
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realize we could use grid! 👍 I didn't know if support was there enough for it.

Copy link
Contributor

@jeryj jeryj left a comment

Choose a reason for hiding this comment

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

It's working really well! Nicely done 👍

@jeherve jeherve added this to the 8.9 milestone Aug 12, 2020
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good to me. 🚢

Comment on lines +93 to +97
description: getEditedPostAttribute( 'excerpt' ) || getEditedPostAttribute( 'content' ),
url: getEditedPostAttribute( 'link' ),
author: user?.name,
image:
!! featuredImageId && getMediaSourceUrl( getMedia( featuredImageId ), getCurrentPostId() ),
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but we'll probably need to adjust this to be more exact; Social Previews are generated from Open Graph Meta Tags and Twitter Meta Tags, and those are not always as simple as checking for a featured image or an excerpt. That depends on what plugin you're using to generate those tags.

In Jetpack and on WordPress.com for example, we go through different steps to find an image, and do not limit ourselves to featured images.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Aug 12, 2020
@marekhrabe marekhrabe merged commit a77a757 into master Aug 12, 2020
@marekhrabe marekhrabe deleted the add/social-preview-modal branch August 12, 2020 11:37
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 12, 2020
@marekhrabe
Copy link
Contributor Author

r212041-wpcom

davidlonjon added a commit that referenced this pull request Aug 14, 2020
* master: (41 commits)
  use blog token to make the request (#16635)
  External Media: Add account disconnect button (#16759)
  CI: Try collect js coverage (#16786)
  Sync: Fix nonce action string in theme edit sync (#16702)
  Connect-in-place: hide new heading during connection process (#16703)
  Update dependency eslint-plugin-jsdoc to v30.2.1 (#16765)
  Theme Tools: Resolve PHP 7.4 array offset notice. (#16795)
  New shell command for easier access to the database. (#16761)
  My Plan: Add Offer Reset project new plans (Jetpack Security, Jetpack Complete) (#16739)
  Increase the `editor.MediaUpload` hook priority (#16669)
  External Media: Remove `speak` announcement when inserting media.
  Extensions: make `render_callback` optional when checking block registration against plan (#16746)
  Conditional check for wrapper before giving focus to new page (#16817)
  Docker: Add package testing shortcut (#16810)
  Settings: Recognize valid Akismet keys from wp-config and restrict input (#16542)
  Social Previews: Add Modal (#16704)
  Update dependency preact to v10.4.7 (#16768)
  Improve a11y of amp-social-share (#16737)
  Instant Search: Tweak expanded result path styling (#16762)
  Docker: Add phpmyadmin to the docker-composer.yml (#16806)
  ...
pereirinha pushed a commit that referenced this pull request Sep 10, 2020
* update social-previews dependency

* add basic modal functionality

* add prop overrides for individual previews

* connect modal to post data

* rewrite upgrade nudge placeholder

* remove unused select

* upgrade nudge compat

* Update extensions/blocks/social-previews/modal.js

Co-authored-by: Damián Suárez <[email protected]>

* [not verified] extracted the upgrade component to avoid future conflicts

* extracted featured image url logic

* use optional chaining

* Update/social preview modal styles (#16745)

* Social Previews: update modal styles

* Social Previews: add icons to modal

* Social Previews: make icon gray

* Social Previews: updates styles for mobile

* Social Previews: refactor modal to inject the icon

Co-authored-by: cpap <[email protected]>

* Use Gutenberg breakpoints

* Remove color-studio dependency in favor of Gutenberg colors

* Remove atoms colors  dependency in favor of Gutenberg colors

* Social Previews: Upgrade Nudge in Modal (#16744)

* add ability for extensions to load images from the jetpack dir

* add temporary override for availability check

* add first iteration of the upgrade nudge

* Update styles

* fix typo

Co-authored-by: Michael P. Pfeiffer <[email protected]>

* change approach for including image

* Restructure styles for improved maintainability

* Fix upgrade nudge modal padding

* removed debug code

* Adjust font sizes in nudge modal

* Change TabPanel orientation to vertical for correct arrow key navigation

Co-authored-by: Damián Suárez <[email protected]>
Co-authored-by: Harris Papazolgou <[email protected]>
Co-authored-by: cpap <[email protected]>
Co-authored-by: Michael P. Pfeiffer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet