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/79113 incorrect subheading style and copy on wc core profiler #39526

Conversation

moon0326
Copy link
Contributor

@moon0326 moon0326 commented Aug 1, 2023

Submission Review Guidelines:

Changes proposed in this Pull Request:

Closes Automattic/wp-calypso#79113

How to test the changes in this Pull Request:

  1. Checkout this branch and build the assets.
  2. Start the core profiler.
  3. Confirm subheading letter-spacing is 0

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Comment

@moon0326 moon0326 requested review from a team, chihsuan and adrianduffell August 1, 2023 23:12
@github-actions github-actions bot added focus: react admin [team:Ghidorah] plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Aug 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

Hi @chihsuan, @adrianduffell, @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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

Test Results Summary

Commit SHA: c8a9423

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202611m 11s
E2E Tests1890019020813m 44s

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.

Copy link
Member

@chihsuan chihsuan left a comment

Choose a reason for hiding this comment

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

@moon0326 Thanks for the fix!

I'm not sure if we want to update the subheading style in the Core. I thought we wanted to fix it on Jetpack connect page since @verofasulo identified this style bug on Jetpack connect page.

Besides, I checked the CSS styles, and I noticed that letter-spacing difference. Core's letter-spacing is 0.25px. Could you also update it? 🙏

Screenshot 2023-08-02 at 13 00 19 Screenshot 2023-08-02 at 13 00 16

@verofasulo
Copy link

Core's letter-spacing is 0.25px.

@moon0326, where do you see this? Letter spacing should be 0px both for body and heading 😅

@moon0326
Copy link
Contributor Author

moon0326 commented Aug 2, 2023

Core's letter-spacing is 0.25px.

@moon0326, where do you see this? Letter spacing should be 0px both for body and heading 😅

@verofasulo Subheadings have 0.25px letter spacing

0.25px:
Screen Shot 2023-08-02 at 4 20 40 PM

0px:
Screen Shot 2023-08-02 at 4 20 45 PM

@moon0326 moon0326 force-pushed the update/79113-incorrect-subheading-style-and-copy-on-wc-core-profiler branch from e52af60 to 2c893d0 Compare August 2, 2023 23:24
@moon0326
Copy link
Contributor Author

moon0326 commented Aug 2, 2023

@moon0326 Thanks for the fix!

I'm not sure if we want to update the subheading style in the Core. I thought we wanted to fix it on Jetpack connect page since @verofasulo identified this style bug on Jetpack connect page.

Besides, I checked the CSS styles, and I noticed that letter-spacing difference. Core's letter-spacing is 0.25px. Could you also update it? 🙏

Screenshot 2023-08-02 at 13 00 19 Screenshot 2023-08-02 at 13 00 16

@chihsuan Ah..gotcha :) I'll open a new PR in wp-calypso 👍

Updated letter-spacing in 2c893d0

Copy link
Member

@chihsuan chihsuan left a comment

Choose a reason for hiding this comment

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

Thanks @moon0326 LGTM and tested well. 👍

I think it's okay to merge this PR with just one approval.

@moon0326 moon0326 force-pushed the update/79113-incorrect-subheading-style-and-copy-on-wc-core-profiler branch from 2c893d0 to c8a9423 Compare August 3, 2023 17:54
@moon0326 moon0326 merged commit 3c07a03 into trunk Aug 3, 2023
23 checks passed
@moon0326 moon0326 deleted the update/79113-incorrect-subheading-style-and-copy-on-wc-core-profiler branch August 3, 2023 18:26
@github-actions github-actions bot added this to the 8.1.0 milestone Aug 3, 2023
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Aug 3, 2023
@lanej0 lanej0 removed the needs: analysis Indicates if the PR requires a PR testing scrub session. label Aug 3, 2023
tommyshellberg pushed a commit that referenced this pull request Aug 7, 2023
…39526)

* Use -webkit-font-smoothing: antialiased for core profiler subheadings

* Remove subheading letter-spacing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect subheading style and copy on the Woo Core Profiler Jetpack screen
4 participants