-
Notifications
You must be signed in to change notification settings - Fork 145
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(pay): ui improvements #4048
Conversation
a674432
to
ae70226
Compare
ecfe853
to
d7c8da1
Compare
rebase rebase
d7c8da1
to
27a15e0
Compare
b9358bf
to
afe3e97
Compare
apps/pay/app/layout.tsx
Outdated
@@ -2,10 +2,10 @@ import type { Metadata } from "next" | |||
import { Inter_Tight } from "next/font/google" | |||
|
|||
// eslint-disable-next-line import/no-unassigned-import | |||
import "./globals.css" | |||
import "bootstrap/dist/css/bootstrap.css" | |||
|
|||
// eslint-disable-next-line import/no-unassigned-import |
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.
// eslint-disable-next-line import/no-unassigned-import | |
// eslint-disable-next-line import/no-unassigned-import |
is this still necessary?
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.
yes, these comments work for next line only. We can add comment to ignore in entire file, but not sure if that is better approach.
@@ -17,26 +16,28 @@ export default function CurrencyDropdown({ | |||
showOnlyFlag?: boolean | |||
}) { | |||
const searchParams = useSearchParams() | |||
const display = searchParams?.get("display") | |||
const display = searchParams?.get("displayCurrency") |
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 would keep display for now. keep the breaking changes / higher risk element for a smaller PR. so we can isolate the effect of it and decide later if that makes sense or not to do it.
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 added support for display
as well, and it was working with URLs with display parameter. However, I am reverting this to make a smaller PR later.
<div className="link-wrapper"> | ||
<a | ||
className={styles.link} | ||
href="https://galoy.io" | ||
target="_blank" | ||
rel="noreferrer" | ||
> | ||
Powered by <GaloyIcon /> | ||
</a> | ||
</div> |
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.
we can remove that. at least as it is currently, it's taking too much real estate for a POS screen (we should not have the ability to scroll on such a screen really, the focus should be on inputing an amount with a keyboard and we should not distract from this action by making the page longer)
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.
This is part for receipt PDF. Not for webpage.
Also, this PR is pretty big. If it's possible to break in down in smaller one, that would ease and probably speed up the review process |
afe3e97
to
80a7ed8
Compare
improvements in UI
In future PRs