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

External Media: Add bottom padding to Google Photos modal #21201

Merged
merged 6 commits into from
Oct 19, 2021

Conversation

obenland
Copy link
Member

@obenland obenland commented Sep 27, 2021

Changes proposed in this Pull Request:

  • Add bottom padding to Google Photos modal to avoid it looking unsightly on large screens.

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

No.

Testing instructions:

  • In Jetpack Beta, switch to this branch and open the Editor.
  • Insert an image block
  • Click in Select Image (or Select Media)
  • Select Google Photos
  • Make sure the authentication modal looks like it does in the After screenshot.
  • Make sure you can still access Pexels photos as well
  • Make sure it looks good on mobile/small screens

Before

Screen Shot 2021-10-11 at 1 14 09 PM

After

Screen Shot 2021-10-11 at 1 11 50 PM

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello obenland! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D67479-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

@github-actions github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Sep 27, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Sep 27, 2021

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ⚠️ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: November 2, 2021.
  • Scheduled code freeze: October 25, 2021.

@github-actions github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Sep 27, 2021
@obenland obenland added [Status] In Progress and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 27, 2021
@obenland obenland marked this pull request as ready for review September 27, 2021 20:08
@obenland obenland added [Extension] External Media Extend all block editor media tools to support external providers [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Sep 27, 2021
@sixhours
Copy link
Contributor

This fixes the bug with Google Photos, but creates a new issue where you can't scroll to the bottom of the Pexels image selection, so you can't load more results.

With this patch:

Screen Shot 2021-09-27 at 5 29 04 PM

On production:

Screen Shot 2021-09-27 at 5 29 46 PM

:not(...) selectors can produce unexpected results (remember the color annotations bug in some themes that changed all the button colors in the editor? 🙃 ) so it might be worth it to use a more specific selector here, if possible.

@obenland
Copy link
Member Author

Nice catch @sixhours! Hm, not sure how to fix this then, it's an odd one. Changing CSS values in Safari seems to make the modal jump back into its usual shape—I wonder if it's a rendering issue?

jeherve
jeherve previously approved these changes Sep 28, 2021
@jeherve jeherve dismissed their stale review September 28, 2021 13:24

Ah yes, I caught the bug @sixhours reported as well.

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 28, 2021
@sixhours sixhours self-assigned this Oct 11, 2021
@sixhours sixhours added [Status] Needs Team Review and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Oct 11, 2021
@sixhours sixhours removed their request for review October 11, 2021 17:18
@sixhours sixhours force-pushed the update/external-media-modal branch 2 times, most recently from 725f174 to 9b5837b Compare October 12, 2021 20:34

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
…ig screens

Fixes a bug where there was no breathing room at the bottom of the modal.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
…re targeting
@sixhours sixhours force-pushed the update/external-media-modal branch from 9b5837b to 8ada054 Compare October 13, 2021 16:34
@sixhours sixhours changed the title External Media: Prevent modal from flexing on big screens. External Media: Add bottom padding to Google Photos modal Oct 14, 2021
Copy link
Contributor

@mreishus mreishus left a comment

Choose a reason for hiding this comment

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

I'm not able to replicate the initial issue, since my modals have 90vh height and stretch down to the bottom of the window, regardless of padding:
2021-10-15_10-01

However, the extra padding looks to be an improvement to me, and I see no harm caused by it:
2021-10-15_10-02

@mreishus mreishus added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Team Review labels Oct 15, 2021
@mreishus mreishus requested a review from jeherve October 15, 2021 15:12
@jeherve jeherve added this to the jetpack/10.3 milestone Oct 19, 2021
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 19, 2021
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 works for me now.

Verified

This commit was signed with the committer’s verified signature.
jeherve Jeremy Herve
…ia-modal
@jeherve jeherve enabled auto-merge (squash) October 19, 2021 07:45
@jeherve jeherve merged commit 8f8c16b into master Oct 19, 2021
@jeherve jeherve deleted the update/external-media-modal branch October 19, 2021 07:58
@github-actions
Copy link
Contributor

Great news! One last step: head over to your WordPress.com diff, D67479-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 19, 2021
davidlonjon added a commit that referenced this pull request Oct 19, 2021
* master:
  Releases: start 10.3-a.4 cycle (#21454)
  Atomic Release: prepare changelogs for today's release (#21453)
  Connection: fixing the bug with connect button not appearing for secondary users (#21450)
  CLI: Add readme to 'jetpack release' (#21418)
  Update handling and setting CCPA related cookies (#21337)
  Publicize: do not disable toggle connection control when RePublicize is enabled (#21373)
  External Media: Add bottom padding to Google Photos modal (#21201)
  Blog Subscription Widget: deprecate widget and transform to block (#21184)
davidlonjon added a commit that referenced this pull request Oct 19, 2021
# By Jeremy Herve (2) and others
# Via GitHub
* master:
  Releases: start 10.3-a.4 cycle (#21454)
  Atomic Release: prepare changelogs for today's release (#21453)
  Connection: fixing the bug with connect button not appearing for secondary users (#21450)
  CLI: Add readme to 'jetpack release' (#21418)
  Update handling and setting CCPA related cookies (#21337)
  Publicize: do not disable toggle connection control when RePublicize is enabled (#21373)
  External Media: Add bottom padding to Google Photos modal (#21201)
  Blog Subscription Widget: deprecate widget and transform to block (#21184)

# Conflicts:
#	projects/plugins/boost/tests/e2e/package.json
@mreishus
Copy link
Contributor

r233557

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Extension] External Media Extend all block editor media tools to support external providers [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants