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

style: APP-393 align dollar sign in small screens #2509

Merged
merged 3 commits into from
Oct 23, 2024

Conversation

r41ph
Copy link
Contributor

@r41ph r41ph commented Oct 21, 2024

Description

https://regennetwork.atlassian.net/browse/APP-393

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • provided a link to the relevant issue or specification
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

How to test

  1. Go to the Buy Credits form (https://deploy-preview-2509--regen-marketplace.netlify.app/project/C01-001/buy) and with USD selected verify that the dollar sign is properly aligned on both mobile and desktop screens.

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

Copy link

netlify bot commented Oct 21, 2024

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit 50ddafa
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/6718d069ddb3f70008392085
😎 Deploy Preview https://deploy-preview-2509--regen-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@r41ph r41ph requested a review from blushi October 21, 2024 16:33
@r41ph
Copy link
Contributor Author

r41ph commented Oct 21, 2024

@erikalogie see testing instructions

@erikalogie
Copy link
Collaborator

LGTM though I think there could be a 2-3 px more space between the $ sign and the number on mobile.

@blushi
Copy link
Member

blushi commented Oct 22, 2024

This doesn't look like on figma where the $ is perfectly aligned with the amount on both desktop and mobile

image

image

@r41ph
Copy link
Contributor Author

r41ph commented Oct 22, 2024

Good catch. In the Order Summary the $ is above the amount and in the currency field is next to the amount.

LGTM though I think there could be a 2-3 px more space between the $ sign and the number on mobile.

@erikalogie Do you want it to look like in Figma or the currency field should look like the Order Summary?

image

@erikalogie
Copy link
Collaborator

I think in the interest of time, how you've currently implemented it is a great improvement and we should just go with this for now and can tweak it more later!

@r41ph r41ph force-pushed the fix-APP-393-layout-dollar-sign branch from a023d3b to 649ab37 Compare October 22, 2024 15:45
@r41ph
Copy link
Contributor Author

r41ph commented Oct 22, 2024

I think in the interest of time, how you've currently implemented it is a great improvement and we should just go with this for now and can tweak it more later!

@erikalogie Ok, I have moved the $ a bit as requested, please have a look

@erikalogie
Copy link
Collaborator

LGTM!

@r41ph r41ph force-pushed the fix-APP-393-layout-dollar-sign branch from d812211 to 50ddafa Compare October 23, 2024 10:31
@r41ph r41ph merged commit b198ebe into dev Oct 23, 2024
2 checks passed
@r41ph r41ph deleted the fix-APP-393-layout-dollar-sign branch October 23, 2024 10:34
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