Skip to content

feat: upgrade playground to new payment-widget version #15

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 15, 2024
Merged

feat: upgrade playground to new payment-widget version #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "rn-checkout",
"version": "0.1.0",
"version": "0.1.1",
"private": true,
"scripts": {
"dev": "next dev",
Expand All @@ -18,7 +18,7 @@
"@radix-ui/react-slot": "^1.1.0",
"@radix-ui/react-switch": "^1.1.0",
"@radix-ui/react-tooltip": "^1.1.2",
"@requestnetwork/payment-widget": "^0.1.2",
"@requestnetwork/payment-widget": "^0.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Missing Release Information for @requestnetwork/payment-widget v0.2.0

The specified version v0.2.0 of @requestnetwork/payment-widget does not exist in the repository, and no release notes or changelog were found. This update may introduce unverified changes or conflicts.

🔗 Analysis chain

Dependency update aligns with PR objectives.

The update of @requestnetwork/payment-widget from ^0.1.2 to ^0.2.0 directly addresses the main objective of the PR to upgrade the playground to the new version.

To ensure this update doesn't introduce any breaking changes, let's check the changelog or release notes:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for breaking changes in @requestnetwork/payment-widget v0.2.0

# Test: Search for changelog or release notes
rg -i -A 10 "changelog|release notes" README.md CHANGELOG.md || echo "No changelog found in README.md or CHANGELOG.md"

# Test: Check GitHub releases for v0.2.0
gh release view v0.2.0 -R RequestNetwork/payment-widget || echo "GitHub release notes for v0.2.0 not found"

Length of output: 474

Script:

#!/bin/bash
# Description: Verify the correct repository name and check for release notes and version tag

# Test: Confirm the correct GitHub repository name
gh repo list | grep -i "payment-widget" || echo "Repository containing 'payment-widget' not found."

# Test: Search for documentation files that might contain release notes
fd "release" --type f --extension md | xargs grep -i "v0.2.0" || echo "No release notes found in documentation files."

# Test: Verify if the version tag v0.2.0 exists in the repository
git tag | grep "^v0.2.0$" || echo "Version tag v0.2.0 does not exist in the repository."

Length of output: 460

"class-variance-authority": "^0.7.0",
"clsx": "^2.1.1",
"cmdk": "^1.0.0",
Expand Down
89 changes: 84 additions & 5 deletions src/components/Playground.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { PropsValidation } from "@/lib/validation";
import { zodResolver } from "@hookform/resolvers/zod";
import PaymentWidget from "@requestnetwork/payment-widget/react";
import { CopyIcon } from "lucide-react";
import { useRef, useState } from "react";
import { useEffect, useRef, useState } from "react";
import { useForm } from "react-hook-form";
import { z } from "zod";
import { Button } from "./ui/button";
Expand All @@ -21,6 +21,9 @@ import {
AccordionTrigger,
} from "./ui/accordion";

import { cn } from "@/lib/utils";
import { ZERO_ADDRESS } from "@/lib/constants";

export const Playground = () => {
const {
register,
Expand All @@ -38,6 +41,8 @@ export const Playground = () => {
buyerInfo: {},
invoiceNumber: "",
enableBuyerInfo: false,
feeAddress: ZERO_ADDRESS,
feeAmount: 0,
},
});

Expand Down Expand Up @@ -65,6 +70,10 @@ export const Playground = () => {
formValues.supportedCurrencies?.length &&
`supportedCurrencies={${JSON.stringify(formValues.supportedCurrencies)}}`,
formValues.invoiceNumber && `invoiceNumber="${formValues.invoiceNumber}"`,
formValues.feeAddress &&
formValues.feeAddress !== ZERO_ADDRESS &&
`feeAddress="${formValues.feeAddress}"`,
formValues.feeAmount && `feeAmountInUSD={${formValues.feeAmount}}`,
Comment on lines +73 to +76
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix omission of feeAmountInUSD when the amount is zero.

The conditional check formValues.feeAmount && will exclude feeAmountInUSD from the generated integration code when feeAmount is 0, because 0 is a falsy value in JavaScript. This might lead to unintended behavior if a fee amount of zero is a valid input.

Apply this diff to ensure feeAmountInUSD is included even when feeAmount is 0:

- formValues.feeAmount && `feeAmountInUSD={${formValues.feeAmount}}`,
+ (formValues.feeAmount !== undefined && formValues.feeAmount !== null) && `feeAmountInUSD={${formValues.feeAmount}}`,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
formValues.feeAddress &&
formValues.feeAddress !== ZERO_ADDRESS &&
`feeAddress="${formValues.feeAddress}"`,
formValues.feeAmount && `feeAmountInUSD={${formValues.feeAmount}}`,
formValues.feeAddress &&
formValues.feeAddress !== ZERO_ADDRESS &&
`feeAddress="${formValues.feeAddress}"`,
(formValues.feeAmount !== undefined && formValues.feeAmount !== null) && `feeAmountInUSD={${formValues.feeAmount}}`,

]
.filter(Boolean)
.join("\n ");
Expand Down Expand Up @@ -99,6 +108,13 @@ const YourComponent = () => {
}
};

useEffect(() => {
if (formValues.feeAddress?.length === 0) {
setValue("feeAddress", ZERO_ADDRESS);
setValue("feeAmount", 0);
}
}, [formValues.feeAddress, formValues.feeAmount]);
Comment on lines +111 to +116
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid unnecessary re-renders by correcting useEffect dependencies.

The useEffect hook depends on both formValues.feeAddress and formValues.feeAmount, but only formValues.feeAddress is used in the effect's logic. Including formValues.feeAmount may cause unnecessary re-renders.

Apply this diff to adjust the dependency array:

- }, [formValues.feeAddress, formValues.feeAmount]);
+ }, [formValues.feeAddress]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
useEffect(() => {
if (formValues.feeAddress?.length === 0) {
setValue("feeAddress", ZERO_ADDRESS);
setValue("feeAmount", 0);
}
}, [formValues.feeAddress, formValues.feeAmount]);
useEffect(() => {
if (formValues.feeAddress?.length === 0) {
setValue("feeAddress", ZERO_ADDRESS);
setValue("feeAmount", 0);
}
}, [formValues.feeAddress]);


return (
<div className="flex flex-col gap-4 mt-4">
<section className="flex flex-col gap-6 lg:gap-4 items-center md:items-start md:justify-between lg:flex-row">
Expand Down Expand Up @@ -359,10 +375,17 @@ const YourComponent = () => {
</div>
{/* Seller Address */}
<div className="flex flex-col gap-2">
<Label>Seller address</Label>
<Label className="flex items-center">
Seller address
<span className="text-red-500 ml-1">*</span>
</Label>
<Input
placeholder="0x1234567890123456789012345678901234567890"
{...register("sellerAddress")}
className={cn(
"border-2",
errors.sellerAddress ? "border-red-500" : "border-gray-200"
)}
/>
{errors.sellerAddress?.message && (
<Error>{errors.sellerAddress.message}</Error>
Expand All @@ -371,12 +394,19 @@ const YourComponent = () => {

{/* Amount in USD */}
<div className="flex flex-col gap-2">
<Label>Amount in USD</Label>
<Label className="flex items-center">
Amount in USD
<span className="text-red-500 ml-1">*</span>
</Label>
<Input
placeholder="25.55"
{...register("amountInUSD", {
valueAsNumber: true,
})}
className={cn(
"border-2",
errors.amountInUSD ? "border-red-500" : "border-gray-200"
)}
/>
{errors.amountInUSD?.message && (
<Error>{errors.amountInUSD.message}</Error>
Expand All @@ -392,10 +422,44 @@ const YourComponent = () => {
)}
</div>

{/* Fee */}
<div className="flex flex-col gap-2">
<Label>Fee Address</Label>
<Input
placeholder="0x1234567890123456789012345678901234567890"
{...register("feeAddress")}
/>
{errors.feeAddress?.message && (
<Error>{errors.feeAddress.message}</Error>
)}
<Label>Fee Amount in USD</Label>
<Input
placeholder="25.55"
{...register("feeAmount", {
valueAsNumber: true,
})}
/>
{errors.feeAmount?.message && (
<Error>{errors.feeAmount.message}</Error>
)}
</div>
Comment on lines +428 to +445
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add conditional error styling to Fee Address and Fee Amount inputs.

The feeAddress and feeAmount inputs currently lack conditional styling to indicate validation errors, which may lead to a less intuitive user experience.

Apply this diff to include error styling:

<Input
  placeholder="0x1234567890123456789012345678901234567890"
  {...register("feeAddress")}
+ className={cn(
+   "border-2",
+   errors.feeAddress ? "border-red-500" : "border-gray-200"
+ )}
/>
...
<Input
  placeholder="25.55"
  {...register("feeAmount", {
    valueAsNumber: true,
  })}
+ className={cn(
+   "border-2",
+   errors.feeAmount ? "border-red-500" : "border-gray-200"
+ )}
/>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<Input
placeholder="0x1234567890123456789012345678901234567890"
{...register("feeAddress")}
/>
{errors.feeAddress?.message && (
<Error>{errors.feeAddress.message}</Error>
)}
<Label>Fee Amount in USD</Label>
<Input
placeholder="25.55"
{...register("feeAmount", {
valueAsNumber: true,
})}
/>
{errors.feeAmount?.message && (
<Error>{errors.feeAmount.message}</Error>
)}
</div>
<Input
placeholder="0x1234567890123456789012345678901234567890"
{...register("feeAddress")}
className={cn(
"border-2",
errors.feeAddress ? "border-red-500" : "border-gray-200"
)}
/>
{errors.feeAddress?.message && (
<Error>{errors.feeAddress.message}</Error>
)}
<Label>Fee Amount in USD</Label>
<Input
placeholder="25.55"
{...register("feeAmount", {
valueAsNumber: true,
})}
className={cn(
"border-2",
errors.feeAmount ? "border-red-500" : "border-gray-200"
)}
/>
{errors.feeAmount?.message && (
<Error>{errors.feeAmount.message}</Error>
)}
</div>


{/* Currencies */}
<div className="flex flex-col gap-2">
<Label>Currencies</Label>
<CurrencyCombobox register={register} name="supportedCurrencies" />
<Label className="flex items-center">
Currencies
<span className="text-red-500 ml-1">*</span>
</Label>
<CurrencyCombobox
register={register}
name="supportedCurrencies"
className={cn(
"border-2",
errors.supportedCurrencies
? "border-red-500"
: "border-gray-200"
)}
/>
<div className="flex items-center gap-2 flex-wrap">
{formValues.supportedCurrencies?.map((currency) => {
return (
Expand All @@ -408,6 +472,9 @@ const YourComponent = () => {
);
})}
</div>
{errors.supportedCurrencies?.message && (
<Error>{errors.supportedCurrencies.message}</Error>
)}
</div>
</div>
<div className="w-full lg:w-1/2">
Expand All @@ -426,6 +493,18 @@ const YourComponent = () => {
// @ts-ignore
supportedCurrencies={formValues.supportedCurrencies}
invoiceNumber={formValues.invoiceNumber}
feeAddress={
formValues.feeAddress && formValues.feeAddress.length > 0
? formValues.feeAddress
: ZERO_ADDRESS
}
feeAmountInUSD={
formValues.feeAddress &&
formValues.feeAddress.length > 0 &&
formValues.feeAddress !== ZERO_ADDRESS
? formValues.feeAmount
: 0
}
/>
</div>
</section>
Expand Down
9 changes: 7 additions & 2 deletions src/components/ui/combobox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,14 @@ const currencies = Object.entries(CURRENCY_ID).map(([key, value]) => ({
interface CurrencyComboboxProps {
register: UseFormRegister<any>;
name: string;
className?: string;
}

export function CurrencyCombobox({ register, name }: CurrencyComboboxProps) {
export function CurrencyCombobox({
register,
name,
className,
}: CurrencyComboboxProps) {
const [open, setOpen] = React.useState(false);
const [selectedCurrencies, setSelectedCurrencies] = React.useState<string[]>(
[]
Expand All @@ -55,7 +60,7 @@ export function CurrencyCombobox({ register, name }: CurrencyComboboxProps) {
variant="outline"
role="combobox"
aria-expanded={open}
className="w-[300px] justify-between"
className={cn("w-[300px] justify-between", className)}
>
{selectedCurrencies.length > 0
? `${selectedCurrencies.length} selected`
Expand Down
1 change: 1 addition & 0 deletions src/lib/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const ZERO_ADDRESS = "0x0000000000000000000000000000000000000000";
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Consider adding a brief comment for clarity.

The addition of the ZERO_ADDRESS constant is a good practice. It provides a centralized definition for the Ethereum zero address, which can be reused throughout the codebase.

Consider adding a brief comment explaining the purpose and significance of the zero address in Ethereum. This would enhance code readability and maintainability. Here's a suggested implementation:

+// The Ethereum zero address, often used to represent burning tokens or as a placeholder
 export const ZERO_ADDRESS = "0x0000000000000000000000000000000000000000";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const ZERO_ADDRESS = "0x0000000000000000000000000000000000000000";
// The Ethereum zero address, often used to represent burning tokens or as a placeholder
export const ZERO_ADDRESS = "0x0000000000000000000000000000000000000000";

9 changes: 9 additions & 0 deletions src/lib/validation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,13 @@ export const PropsValidation = z.object({
.default([]),
invoiceNumber: z.string().optional(),
enableBuyerInfo: z.boolean().default(false),
feeAddress: z
.string()
.refine(isEthereumAddress, "Invalid fee address")
.optional()
.nullable(),
Comment on lines +65 to +69
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM with a minor suggestion.

The feeAddress property is well-defined and aligns with the PR objectives. The use of isEthereumAddress for validation is appropriate.

Consider adding a comment explaining the purpose of the feeAddress field for better code documentation. For example:

// The Ethereum address where the fee will be sent
feeAddress: z
  .string()
  .refine(isEthereumAddress, "Invalid fee address")
  .optional()
  .nullable(),

feeAmount: z
.number()
.gte(0, "Fee amount needs to be higher than 0")
.default(0),
Comment on lines +70 to +73
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM with minor suggestions.

The feeAmount property is well-defined and aligns with the PR objectives. The use of gte(0) ensures a non-negative fee amount.

Consider the following improvements:

  1. Add a comment explaining the purpose of the feeAmount field:
// The amount of the fee to be applied
feeAmount: z
  .number()
  .gte(0, "Fee amount must be 0 or higher")
  .default(0),
  1. Update the error message to be consistent with the validation:
    Change "Fee amount needs to be higher than 0" to "Fee amount must be 0 or higher".

These changes will improve code documentation and make the error message more accurate.

});