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

Update JP block style consistency after core update #14371

Merged
merged 19 commits into from
Jan 17, 2020

Conversation

davemart-in
Copy link
Contributor

Description

One of the effects of the merge of this core PR is that blank slate screens go from being center aligned to being left aligned.

@huguesvincent raised a flag when he noticed that many of the JP blocks now look off visually.

The goal of this PR is to reign in the blank slate screen design across all JP blocks.

Testing Instructions

  • Set up a local JP dev environment
  • Make sure Gutenberg plugin is also installed
  • Test each block listed in the change log below

Change Log

Here's a detailed list of changes I'm made including before/after screenshots for each update:

Contact Form

Before:

After:

Changes:

  • Replaced top <p> tag with placeholder__instructions to reduce amount of padding between elements.
  • Reduced bottom margin of help-message.
  • Lightened up the help-message text to give everything a touch more visual hierarchy
  • Adjusted margin around extra line of text and button

Gif

Before:

After:

Changes:

  • Removed margin:0 auto; which was forcing the input to be centered
  • Removed isLarge CSS property to button
  • Added isSecondary CSS property to button
  • Adjusted height of input to match button height

Note:

I left a not on this core issue to see if we can get the input and button height issue addressed in core. If that happens, I'll circle back and remove my extra input height CSS here.

MailChimp

Before:

After:

Changes:

  • Moved instructions to instructions prop
  • Removed
    tags

Map

Before:

After:

Changes:

  • Moved instructions to instructions prop
  • Removed
    tags
  • Added a bit more structure
  • Added isSecondary to button

Pinterest

Before:

After:

Changes:

  • Added isSecondary to button

Recurring Payments

Before:

After:

Changes:

  • Moved instructions to instructions prop
  • Removed some unnecessary CSS

Repeat Visitor

Before:

After:

Changes:

  • Added margin between label and input
  • Removed some unnecessary margin-bottom

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello davemart-in! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D37803-code before merging this PR. Thank you!

@jetpackbot
Copy link

jetpackbot commented Jan 16, 2020

Warnings
⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

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 🤖

Generated by 🚫 dangerJS against 36f34c4

@folletto
Copy link
Contributor

Design wise 👍

@jeherve jeherve added [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Status] Needs Design Review Design has been added. Needs a review! labels Jan 16, 2020
@jeherve jeherve added this to the 8.2 milestone Jan 16, 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.

I tested this on a site running WordPress 5.2 and no Gutenberg plugin, since we have to support those types of setup as well. I found a few issues, listed below.

I think it'd be nice if we could support both Gutenberg users (i.e. Atomic site owners) as well as the majority of Jetpack site owners that do not use the Gutenberg plugin.

extensions/blocks/gif/edit.js Outdated Show resolved Hide resolved
extensions/blocks/mailchimp/edit.js Show resolved Hide resolved
@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 Jan 16, 2020
@matticbot
Copy link
Contributor

davemart-in, Your synced wpcom patch D37803-code has been updated.

3 similar comments
@matticbot
Copy link
Contributor

davemart-in, Your synced wpcom patch D37803-code has been updated.

@matticbot
Copy link
Contributor

davemart-in, Your synced wpcom patch D37803-code has been updated.

@matticbot
Copy link
Contributor

davemart-in, Your synced wpcom patch D37803-code has been updated.

@davemart-in
Copy link
Contributor Author

Another round of updates:

MailChimp

  • Added padding in-between block icon and title
  • Made button isDefault (note: for some reasons, isSecondary does not work on this block button)
  • I consulted @ItsJonQ and ended up having to change up the display property on the fields div in order to make the "Re-check Connection" link drop properly to the next line and work both with and without the Gutenberg plugin.

Contact Form

  • Changed button from isPrimary to isSecondary to match other buttons
  • Put max-width on information div

Gif

  • Added padding in-between block icon and title
  • Made button isDefault (note: for some reasons, isSecondary does not work on this block button)

Repeat Visitor

  • I updated the alert box text since it was affected by the dark theme.

Visual Preview

Here's a comparison of each of the blocks without Gutenberg, with Gutenberg, and with a dark theme:

preview

Dave Martin added 2 commits January 17, 2020 10:25
- Removed information dive max-width
- Made buttons isDefault (note: for some reasons, isSecondary does not
work on either of these blocks)
@scruffian scruffian force-pushed the update/jp-block-style-consistency branch from 44794b4 to 1d324a3 Compare January 17, 2020 10:25
@matticbot
Copy link
Contributor

davemart-in, Your synced wpcom patch D37803-code has been updated.

@matticbot
Copy link
Contributor

davemart-in, Your synced wpcom patch D37803-code has been updated.

jeherve
jeherve previously approved these changes Jan 17, 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.

I think things look a lot better now. The only issue left seems to be with the GIF block, as discussed above.

Edit: annnd.. I just spotted @scruffian's changes as I was posting this :) I'll check again

@matticbot
Copy link
Contributor

davemart-in, Your synced wpcom patch D37803-code has been updated.

@matticbot
Copy link
Contributor

davemart-in, Your synced wpcom patch D37803-code has been updated.

@scruffian
Copy link
Member

I made a few changes to the map block to remove some code we don't need:
Screenshot 2020-01-17 at 11 04 32

Copy link
Member

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

I made a few comments, but this is looking much better :)

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.

We seem to be in a good spot right now; the GIF search button is still not perfect, but everything else is better than when we started.

@jeherve jeherve removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jan 17, 2020
@folletto
Copy link
Contributor

The changes happened from my initial design review above seem to have tweaked it a little more, so still all good 👍

@folletto folletto removed the [Status] Needs Design Review Design has been added. Needs a review! label Jan 17, 2020
@jeherve jeherve added the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 17, 2020
@davemart-in davemart-in merged commit 70b7b0d into master Jan 17, 2020
@davemart-in davemart-in deleted the update/jp-block-style-consistency branch January 17, 2020 14:55
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 17, 2020
@jeherve
Copy link
Member

jeherve commented Jan 17, 2020

r201783-wpcom

jeherve added a commit that referenced this pull request Jan 27, 2020
jeherve added a commit that referenced this pull request Jan 28, 2020
* [not verified] Remove empty readme section

* Initial changelog for 8.2

* Changelog: add #14220

* Changelog: add #14252

* Changelog: add #14291

* Changelog: add #14309

* Changelog: add #14304

* Changelog: add general connection log.

* Changelog: add #14275

* Changelog: add #14313

* Changelog: add #14213

* Changelog: add #14357

* Add sync testing instructions

* Add 8.1.1 changelog back

See eeaafab and 61757eb

* Changelog: add #14371

* Changelog: add #14386

* Changelog: add #14471

* Changelog: add #14325

* Changelog: add #14194

* Changelog: add #14340

* Changelog: add #14418

* Changelog: add #14417

* Changelog: add #14075

* Changelog: add #14467

* Changelog: add #14307

* Changelog: add #14326
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants