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 Jetpack button and card styles to match WordPress 5.3 #13729

Merged
merged 4 commits into from
Oct 24, 2019

Conversation

gravityrail
Copy link
Contributor

This is just an experiment I did to make the Jetpack buttons and borders look a bit closer to the "flat" styles coming in WordPress 5.3. They may, as a result, look a bit out of place on older versions of WordPress, but they already don't match those versions, so I think this is a net win.

I am not a designer, so I was really just eyeballing this and trying to make it look "closer", not perfect.

This also lays the groundwork for integrating @wordpress/components into our Jetpack admin pages without a style mismatch.

Changes proposed in this Pull Request:

  • Update Jetpack styles to more closely match WordPress 5.3 styles.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Just slightly modifies styles

Proposed changelog entry for your changes:

  • Update UI styles to fit in WordPress 5.3 core components

Screenshots

Here's a cropped before/after shot.

Before:

myplan-current-cropped

After:

myplan-53-cropped

A gallery of the WP 5.3 dash/posts, plus Jetpack before/after screenshots, is available here:

https://cloudup.com/czXdFjw6ykm

@gravityrail gravityrail requested a review from a team as a code owner October 11, 2019 19:08
@github-actions github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Oct 11, 2019
@jetpackbot
Copy link

jetpackbot commented Oct 11, 2019

Warnings
⚠️ "Testing instructions" are missing for this PR. Please add some

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 b8890a5

@gravityrail gravityrail added [Status] Needs Design Review Design has been added. Needs a review! Admin Page React-powered dashboard under the Jetpack menu labels Oct 11, 2019
keoshi
keoshi previously requested changes Oct 14, 2019
Copy link
Contributor

@keoshi keoshi 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 looking good! Below are some of the things I've noticed:

Set up Jetpack button

Not sure exactly what's the scope here, so don't know if this applies, but the green Jetpack button in the initial pre-connection banner has a border that the primary buttons in 5.3 don't (or are the same color as the bg to be exact). It also has the existing Jetpack :active styles, which make it look weird.

jp-connect-button

Buttons

:focus styles don't match what's in 5.3. Same goes for default buttons.

image

image

box-shadow:
    0 0 0 1px #5b9dd9,
    0 0 2px 1px rgba(30, 140, 190, 0.8);

:active styles don't match what's in 5.3. Same goes for default buttons.

image

image

background: #00669B

Double borders

At least the Site Stats block appears to have a double border. It seems to be caused by a parent and child elements where both have a border property.

image

Border radii

The change in border radius seems to have some side effects, where adjacent cards have an undesired dip inside where they should be flush instead.

image

Margins

Some change in margins makes the external link detached from its card.

image

* fixed margins, border radius, visual display bugs, matched focus styles
@MichaelArestad
Copy link
Contributor

I did some pretty major changes:

  • Updated button focus styles (only on primary/secondary)
  • Removed the border radius on card
  • Updated border colors on cards via box shadow (to avoid recoding everything)
  • Updated card border colors on non-card components to match
  • fixed a stats display bug (no more gap)

Suggestions for future updates and migration to WordPress components:

  • One PR to import the components - then merge it in
  • One PR per component - perhaps even more focused

I apologize for whoever has to review this, but it's ready for another review.

@MichaelArestad
Copy link
Contributor

Scratch that. Found a bunch more styles that need updating in signup flows etc.

@MichaelArestad
Copy link
Contributor

MichaelArestad commented Oct 17, 2019

Should be good now. Items to review:

  • Cards
  • Buttons
  • ButtonGroups
  • Jetpack masthead
  • Jetpack dashboard borders and buttons
  • Jetpack settings
  • Disconnected states
  • JITM borders
  • Settings group borders and buttons

@gravityrail
Copy link
Contributor Author

gravityrail commented Oct 18, 2019

Button style on the connection iframe doesn't match Jetpack's new style:

approve-button-style

Since this is rendered on WPCOM, we could switch out button styles based on the jetpack_version flag passed in via the iframe URL.

cc @Automattic/poseidon - this should be opened as a separate issue, not part of this PR.

@gravityrail
Copy link
Contributor Author

How do we feel about the iridescent green setup banner in this new style?

Might be out of scope for this ticket since the color is an unrelated style change.

setup-banner

@gravityrail
Copy link
Contributor Author

Consider this comment a 👍 on this change (I can't leave an actual approval since I still own the PR)

@gravityrail
Copy link
Contributor Author

@keoshi can you take another look and verify that @MichaelArestad fixed the issues you saw?

@MichaelArestad
Copy link
Contributor

How do we feel about the iridescent green setup banner in this new style?

We're going to leave it. At least for this PR.

@joanrho
Copy link
Contributor

joanrho commented Oct 18, 2019

@MichaelArestad Tested all of the following on WordPress 5.3-RC1-46561 running Twenty Twenty/twentytwenty-master theme:

  • Cards
  • Buttons
  • ButtonGroups
  • Jetpack masthead
  • Jetpack dashboard borders and buttons
  • Jetpack settings
  • Disconnected states
  • JITM borders
  • Settings group borders and buttons

These are all looking great, but I caught a few minor things:

  1. "Show me" button text on the Stats page is a bit off (See Stats page: fix misaligned text in "Show Me" button #13783)
  2. dops-section-header height alignment could be evened out (See Admin page: fix misaligned dops section header heights in Jetpack Dashboard cards #13781)
  3. (might be a core design issue) The border-radius of the "Add New" button on the tops of the Posts, Media, Pages, Themes, Plugins, pages is 2px whereas the styles everywhere else in 5.3 all are border-radius: 3px. Screenshot:

Screen Shot 2019-10-18 at 2 42 35 PM

@keoshi
Copy link
Contributor

keoshi commented Oct 18, 2019 via email

@MichaelArestad MichaelArestad added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Design Review Design has been added. Needs a review! labels Oct 22, 2019
@MichaelArestad MichaelArestad added this to the 7.9 milestone Oct 22, 2019
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, but we'll need to add some of those CSS files back.

I also have a few other questions and remarks, but I don't think any of them are blockers.


Should the non-active color be bright blue like this?

image
image

It does not seem in line with the rest of the CSS in wp-admin:

image


It seems there is a bit of padding missing here:
image


Maybe that's just me, but it feels odd to go from the closed dropdown with shadows to the flat borders of the open dropdown:

Screen Recording 2019-10-23 at 02 21 PM


The blue border around the input fields seem to be bright blue. Is that the same color as Core?

image

@@ -1,479 +0,0 @@
/*!
Copy link
Member

Choose a reason for hiding this comment

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

We'll need those files on RTL sites, so I don't think we can delete them. If you do, you get 404 errors when we try to enqueue the file in wp-admin on sites using Hebrew or another RTL language.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. I thought these were generated during build so we didn't want to sync them. I didn't specifically remove this one (at least not intentionally). I'll add it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added

@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 Crew. Label will be renamed soon. labels Oct 23, 2019
@jeherve jeherve mentioned this pull request Oct 23, 2019
11 tasks
@MichaelArestad MichaelArestad added [Status] Needs Review To request a review from Crew. Label will be renamed soon. 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 23, 2019
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.

Looking good now. Merging.

@jeherve jeherve dismissed keoshi’s stale review October 24, 2019 09:45

This was addressed in the comments

@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 Crew. Label will be renamed soon. labels Oct 24, 2019
@jeherve jeherve merged commit f82725b into master Oct 24, 2019
@jeherve jeherve deleted the try/wp-admin-style branch October 24, 2019 09:46
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 24, 2019
@jeherve jeherve added [Status] Has Changelog and removed [Status] Needs Package Release This PR made changes to a package. Let's update that package now. [Status] Needs Changelog labels Oct 24, 2019
jeherve added a commit that referenced this pull request Oct 28, 2019
jeherve added a commit that referenced this pull request Oct 28, 2019
jeherve added a commit that referenced this pull request Oct 29, 2019
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants