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

Use flexbox gap to position currency symbol with the tax information string #98943

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

billrobbins
Copy link
Contributor

@billrobbins billrobbins commented Jan 27, 2025

Follow-up to #98920

Proposed Changes

The new Billing History component based on DataViews (currently enabled in DEV) had a bug where the currency symbol would appear next to the tax string in currency-on-right locales. The lack of space made it look like the currency symbol was part of the string instead of being separated by a space.

This PR adjusts the fix from 98920 to use the flexbox gap property instead of a left-hand margin. It also moves the changes to client/me/purchases/billing-history/style-data-view.scss so they'll be scoped specifically for the new Billing History component.

It also replaces a min-height property for the loading state of the list with a bit of styling to center the spinner without using such an artificially large loading area.

Why are these changes being made?

By utilizing the gap property, we should be able to avoid any unexpected alignment issues if this line were to end up broken up in the future.

Symbol on the left of amount
Screenshot 2025-01-27 at 9 30 09 AM

Symbol on the right of amount
Screenshot 2025-01-27 at 9 20 43 AM

RTL language
Screenshot 2025-01-27 at 9 29 02 AM

Testing Instructions

  • Open http://calypso.localhost:3000/me/purchases/billing and locate a receipt that includes tax.
  • Make sure the currency symbol and tax information are properly aligned.
  • Switch your locale to Greek or Polish and refresh the Billing History. Make sure the alignment includes a space between the currency symbol and the tax string.
  • Switch your locale to a RTL language (such as Hebrew) and check again to make sure the spacing is consistent.

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@billrobbins billrobbins self-assigned this Jan 27, 2025
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@billrobbins billrobbins marked this pull request as ready for review January 27, 2025 17:39
@billrobbins billrobbins requested a review from a team as a code owner January 27, 2025 17:39
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 27, 2025
Copy link
Member

@sirbrillig sirbrillig left a comment

Choose a reason for hiding this comment

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

Looks good!

@billrobbins billrobbins merged commit df56a5b into trunk Jan 29, 2025
16 checks passed
@billrobbins billrobbins deleted the correct-tax-inclusive-price-spacing-in-history branch January 29, 2025 14:44
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants