-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: APP-267 accounts orders page #2483
Conversation
✅ Deploy Preview for regen-website ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@erikalogie see testing instructions |
@r41ph you did an awesome job with this! It really looks great, just a few little tiny details. |
da00326
to
a6b6ece
Compare
@erikalogie thanks! Please review the style updates addressing your comments. |
@r41ph looks awesome! |
@erikalogie have another look when you get a chance please |
web-components/src/components/inputs/new/EditableInput/EditableInput.test.tsx
Show resolved
Hide resolved
const { credits, price, denom } = data as CreditsData; | ||
const totalPrice = +credits * +price; | ||
return ( | ||
<section className="flex gap-20"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of always repeating this same structure we could have a reusable component for all these sections that could take a title, icon and some row items as props
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I think creating a reusable component for these sections may add unnecessary complexity because the logic for each component is slightly different. While they share some structure (title, icon, rows), combining them into a single component would require a few conditional checks that would make them harder to work with. I believe that keeping them separated while using shared internal components allows each component to be simple to maintain and update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then what about just having a reusable component with title, icon and children, which makes it more flexible?
web-marketplace/src/components/organisms/Order/Order.MakeAnonymous.tsx
Outdated
Show resolved
Hide resolved
web-marketplace/src/components/organisms/Order/Order.MakeAnonymous.tsx
Outdated
Show resolved
Hide resolved
web-marketplace/src/components/organisms/Order/Order.SummaryRow.tsx
Outdated
Show resolved
Hide resolved
@r41ph looks great except now the left-alignment got messed up a bit: |
@erikalogie please have another look |
so nice! LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the font for those labels is supposed to be Muli, not Lato, see https://www.figma.com/design/BTuUv6QXY4GbliZcXCe8RJ/Fiat-payments?node-id=3234-100679&node-type=instance&m=dev
I believe we could potentially reuse our TextButton
component (getting rid of the hovering changing color) and this is how it's defined in figma as well
4d6e3f3
to
c04c4c6
Compare
web-marketplace/src/components/organisms/Order/Order.Summary.tsx
Outdated
Show resolved
Hide resolved
web-marketplace/src/components/organisms/Order/Order.Summary.tsx
Outdated
Show resolved
Hide resolved
web-marketplace/src/components/organisms/Order/Order.MakeAnonymous.tsx
Outdated
Show resolved
Hide resolved
web-marketplace/src/components/organisms/Order/Order.SummaryRow.tsx
Outdated
Show resolved
Hide resolved
web-marketplace/src/components/organisms/Order/Order.Summary.tsx
Outdated
Show resolved
Hide resolved
web-marketplace/src/components/organisms/Order/Order.Summary.tsx
Outdated
Show resolved
Hide resolved
d4238ef
to
16d5609
Compare
And the other one: "Credits delivered", is this alright in plural? |
Yes this is good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name: 'editable-credits', | ||
}); | ||
fireEvent.change(editInput, { target: { value: 7 } }); | ||
if (editInput) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like editInput
is always null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch, thanks
…er updating findDisplayDenom based on APP-204
d0ec96e
to
d235204
Compare
Description
https://regennetwork.atlassian.net/browse/APP-267
Author Checklist
I have...
How to test
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...