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

GIF Block: Placeholder State #30402

Merged
merged 9 commits into from
Jan 25, 2019
Merged

GIF Block: Placeholder State #30402

merged 9 commits into from
Jan 25, 2019

Conversation

jeffersonrabb
Copy link
Contributor

Changes proposed in this Pull Request

Initial state of the block is now a Placeholder component and there is no longer a default Giphy image. This brings the block in line with Gutenberg design patterns.

This commit to Jetpack repo is part of the change: Automattic/jetpack@bd0a281

Testing instructions

  • Spin up JN instance: https://jurassic.ninja/create?gutenpack&calypsobranch=fix/gif-placeholder&branch=try/giphy
  • Connect Jetpack.
  • Navigate to Settings->Jetpack Constants and enable JETPACK_BETA_BLOCKS
  • Add GIF block. Verify that default state is the Placeholder.
  • Type in the field, verify the placeholder is replaced by a GIF.
  • Preview or Publish versions of the block with and without an image. If the block is still in Placeholder state the block should be completely suppressed in Publish/Preview.

@matticbot
Copy link
Contributor

@jeffersonrabb jeffersonrabb added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Goal] Gutenberg Working towards full integration with Gutenberg Giphy labels Jan 24, 2019
@jeffersonrabb jeffersonrabb self-assigned this Jan 24, 2019
Copy link
Contributor

@thomasguillot thomasguillot left a comment

Choose a reason for hiding this comment

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

  1. The form doesn't give me enough time to type what I'm looking for before displaying a GIF. See screen recording: https://cloudup.com/cagftUXpnfT

  2. The form's width is not consistent between the placeholder and the iframe:

screenshot 2019-01-25 at 07 29 25

  1. I find it really strange to have a text-align: center on the input text. It's not consistent with the other inputs in Gutenberg.

@sirreal
Copy link
Member

sirreal commented Jan 25, 2019

Good points @thomasguillot 👍

I've made a few changes:

  • Increase automatic search wait time from 250ms to 300ms
  • Match placeholder input max-width (400px)
  • Match existing padding for consistency
  • Left align inputs

Here's what it looks like now, with Form placeholder below for reference:

screen shot 2019-01-25 at 09 31 21

@sirreal sirreal added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 25, 2019
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

👍 This is in a good place, I'll merge it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Gutenberg Working towards full integration with Gutenberg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants