Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: APP-588 layout alignment improvements #2627

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

r41ph
Copy link
Contributor

@r41ph r41ph commented Mar 3, 2025

Description

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

Author Checklist

  • 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. Verify that the Order Summary Card elements align as per the JIRA issue and Figma designs.

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 Mar 3, 2025

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit 1d9a341
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/67d0106adc8a19000831951e
😎 Deploy Preview https://deploy-preview-2627--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.

Copy link

netlify bot commented Mar 3, 2025

Deploy Preview for terrasos ready!

Name Link
🔨 Latest commit 1d9a341
🔍 Latest deploy log https://app.netlify.com/sites/terrasos/deploys/67d0106a8784160008290e05
😎 Deploy Preview https://deploy-preview-2627--terrasos.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 March 3, 2025 12:58
@r41ph
Copy link
Contributor Author

r41ph commented Mar 3, 2025

@erikalogie @S4mmyb see testing instructions

Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

};
export const PrevNextButtons = ({
onPrev,
onSave,
saveDisabled,
saveText,
className = '',
Copy link
Member

Choose a reason for hiding this comment

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

do we really need the default ''

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that doesn’t need to be there. Thanks for pointing it out.

Copy link
Contributor Author

@r41ph r41ph Mar 5, 2025

Choose a reason for hiding this comment

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

The next button needs to be larger, see design:

Good catch. That wasn't even directly mentioned in the issue! Changing this will also change the look of the buttons in the create project flow footer, but they should look the same as this ones anyway, so I'm going to go ahead an update it.

@r41ph r41ph force-pushed the fix-APP-588-buy-credits-style-issues-mobile branch from 4b38ac2 to 49b378f Compare March 5, 2025 14:31
@r41ph r41ph requested a review from blushi March 5, 2025 14:41
@erikalogie
Copy link
Collaborator

This deploy preview is blank for me, in incognito mode as well: https://deploy-preview-2627--regen-marketplace.netlify.app/
Screenshot 2025-03-10 at 10 14 16 AM

@r41ph r41ph force-pushed the fix-APP-588-buy-credits-style-issues-mobile branch from 49b378f to 9896001 Compare March 10, 2025 15:27
@r41ph
Copy link
Contributor Author

r41ph commented Mar 10, 2025

This deploy preview is blank for me, in incognito mode as well..

Try again please

@erikalogie
Copy link
Collaborator

This deploy preview is blank for me, in incognito mode as well..

Try again please

It is working now, thanks!

@erikalogie
Copy link
Collaborator

Everything look much better, just one small thing:
Monosnap Regen Marketplace : Buy, trade and retire carbon credits | Regen Marketplace : Buy, trade and retire carbon credits 2025-03-10 11-55-08

The space between the words "project", "avg price per credit", and "# credits" should all be the same (5px in comps). The space beneath the "# credits" line and the top of the "USD" symbol should also be the same (10px in comps).

@r41ph r41ph force-pushed the fix-APP-588-buy-credits-style-issues-mobile branch from 9896001 to 789c3d6 Compare March 10, 2025 16:35
@r41ph
Copy link
Contributor Author

r41ph commented Mar 10, 2025

The space between the words "project", "avg price per credit", and "# credits" ...

I've adjusted the spacing between elements. The gap between #credits, the avg price, and the separator was larger because the input field on edit is taller than the content when the input is hidden. I checked the comps and maybe that's expected? Just noting that now there's a slight jump when the input appears.

className={classes.btn}
className={cn(
classes.btn,
'!py-[13px] sm:!py-[17px] !px-[30px] sm:!px-[43px] h-50 sm:h-60',
Copy link
Member

Choose a reason for hiding this comment

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

should we rework classes.btn (which has some padding defined that we overwrite here) or just remove it from this className and use tailwind classes entirely, instead of using !important here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. See updated code.

@erikalogie
Copy link
Collaborator

Monosnap Regen Marketplace : Buy, trade and retire carbon credits | Regen Marketplace : Buy, trade and retire carbon credits 2025-03-10 16-07-56

Sorry to be annoying, if you take off the margin top and bottom somehow from the horizontal line, I think that looks better

@r41ph
Copy link
Contributor Author

r41ph commented Mar 11, 2025

Sorry to be annoying, if you take off the margin top and bottom somehow from the horizontal line, I think that looks better

Not at all! The horizontal line doesn’t have any top or bottom margin, I actually can't see the difference. Did you make any other changes that could have affected the layout? The horizontal line is wrapped by an element with a padding-top: 10px. I tried removing it but the line ends up too close to the '#credits', not like in your latest screeshot.

@r41ph r41ph force-pushed the fix-APP-588-buy-credits-style-issues-mobile branch from 789c3d6 to 1d9a341 Compare March 11, 2025 10:28
@erikalogie
Copy link
Collaborator

Sorry to be annoying, if you take off the margin top and bottom somehow from the horizontal line, I think that looks better

Not at all! The horizontal line doesn’t have any top or bottom margin, I actually can't see the difference. Did you make any other changes that could have affected the layout? The horizontal line is wrapped by an element with a padding-top: 10px. I tried removing it but the line ends up too close to the '#credits', not like in your latest screeshot.

This is all I did. Not saying that's how you should accompish this spacing this way, just that it looks better.

Screen.Recording.2025-03-11.at.9.15.17.AM.mov

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants