Skip to content

Various UI fixes in Core profiler steps #38759

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.

Sign up for GitHub

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

Jump to bottom
Merged
ilyasfoo merged 12 commits into from
Jun 17, 2023
Merged

Various UI fixes in Core profiler steps #38759

ilyasfoo merged 12 commits into from
Jun 17, 2023

Conversation

@ilyasfoo
Copy link
Contributor

@ilyasfoo ilyasfoo commented Jun 16, 2023 •
edited
Loading

Changes proposed in this Pull Request:

Closes #38731.

Fixes the following:

  1. Heading letters spacing
  2. Subheading color to the proper grey
  3. Removed duplicated #FunWooFact during Extending your store's capabilities loading message
  4. Vertically center the "Installed" label in the extensions screen
  5. Button color in the geolocation message

Additional changes:

  1. Standardize checkbox styling
  2. Fixed heading size for Woo

Note:

  • I wasn't able to fully match the font for the heading and subheading fonts since it uses SF Pro font, which we have no web license for. Instead, I adjusted the font weight, line height, and letter spacing.

How to test the changes in this Pull Request:

  1. Install and activate WooCommerce on a brand new site
  2. Install the WooCommerce Beta Tester plugin
  3. Go to /wp-admin/tools.php?page=woocommerce-admin-test-helper
  4. Enable the 'core-profiler' feature flag
  5. Visit your public URL /wp-admin/admin.php?page=wc-admin&path=%2Fsetup-wizard
  6. Go through the new core profiler flow.
  7. Observe the heading letter spacing is similar to design
  8. Have at least one extension of the plugins step already installed and activated
  9. Observe the Installed label is now vertically centered

Before:
image

After:
image

Before:
image
After:
image

Before:
image
After:
image

Before:
image
After:
image

@ilyasfoo ilyasfoo requested review from a team, moon0326 and rjchow June 16, 2023 04:51
@github-actions github-actions bot added focus: react admin [team:Ghidorah] plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Jun 16, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 16, 2023

Hi @moon0326, @rjchow, @woocommerce/ghidorah

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@ilyasfoo
Copy link
Contributor Author

ilyasfoo commented Jun 16, 2023

Hi @verofasulo. I wasn't able to use SF Pro directly as a web font since it's a built-in Apple system font. I tried to adjust the font styles to match the design as best I could. You can refer to the first screenshot for comparison, but let me know if you think we should change anything.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 16, 2023 •
edited
Loading

Test Results Summary

Commit SHA: 3963e38

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202611m 13s
E2E Tests1980010020817m 50s
To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

adrianduffell
adrianduffell approved these changes Jun 16, 2023
View reviewed changes
Copy link
Contributor

@adrianduffell adrianduffell 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 looking great, thanks @ilyasfoo! 🚀

@adrianduffell
Copy link
Contributor

adrianduffell commented Jun 16, 2023

Just looks like there is a lint issue:

stylelint '**/*.scss'
plugins/woocommerce-admin lint: client/core-profiler/components/heading/style.scss
plugins/woocommerce-admin lint:  13:19  ✖  Expected a leading zero  number-leading-zero
plugins/woocommerce-admin lint:  ELIFECYCLE  Command failed with exit code 2.

rjchow
rjchow approved these changes Jun 16, 2023
View reviewed changes
Copy link
Contributor

@rjchow rjchow left a comment

Choose a reason for hiding this comment

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

LGTM aside from the lint issue! 🚢
Pre-approving

@ilyasfoo
Copy link
Contributor Author

ilyasfoo commented Jun 16, 2023 •
edited
Loading

I've also removed box shadow borders from checkboxes:

image image

@verofasulo
Copy link

verofasulo commented Jun 16, 2023

I wasn't able to use SF Pro directly as a web font since it's a built-in Apple system font.

@ilyasfoo, thanks for bringing this up! I had no idea. All the designs across Woo are based on SF Pro Text, but if I understood correctly, we use another font for implementation — could you please let me know what we use in the code? Thank you 🙏

@ilyasfoo
Copy link
Contributor Author

ilyasfoo commented Jun 16, 2023

@verofasulo The main font in WC is -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, Oxygen-Sans, Ubuntu, Cantarell, "Helvetica Neue", sans-serif;

@ilyasfoo ilyasfoo merged commit b490e56 into trunk Jun 17, 2023
@ilyasfoo ilyasfoo deleted the fix/38731-core-profiler-ui-fixes branch June 17, 2023 05:14
@github-actions github-actions bot added this to the 7.9.0 milestone Jun 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@moon0326 moon0326 Awaiting requested review from moon0326

2 more reviewers

@adrianduffell adrianduffell adrianduffell approved these changes

@rjchow rjchow rjchow approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Milestone

7.9.0

Development

Successfully merging this pull request may close these issues.

Core profiler UI issues from design call

5 participants

@ilyasfoo @adrianduffell @verofasulo @rjchow