feat: Add Type-to-Search and Icons for Payment Chain and Currency in Create Invoice Form #227
Conversation
…170-allow-type-to-search-for-payment-chain-and-payment-currency-on-create-invoice-form
…170-allow-type-to-search-for-payment-chain-and-payment-currency-on-create-invoice-form
…170-allow-type-to-search-for-payment-chain-and-payment-currency-on-create-invoice-form
WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
sstefdev
requested review from
MantisClone,
aimensahnoun and
rodrigopavezi
and removed request for
aimensahnoun
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
shared/icons/network/ronin.svelte (1)
15-22: Consider using more semantic class names.While the 'r' suffix helps prevent collisions, consider using more semantic class names that better describe their purpose, such as
.ronin-primaryand.ronin-gradient. This would make the styles more maintainable and their purpose more clear.- .st0r { + .ronin-primary { fill-rule: evenodd; clip-rule: evenodd; fill: #004de5; } - .st1r { + .ronin-gradient { fill: url(#SVGID_1_); }Remember to update the class references in the path elements accordingly:
- class="st0r" + class="ronin-primary" - class="st1r" + class="ronin-gradient"packages/create-invoice-form/src/lib/invoice/form.svelte (2)
430-430: Localize the hardcoded "Unknown" label for internationalization.The string
"Unknown"used in the currency label is hardcoded, which may not be suitable for international users.Consider using a localization function:
label: `${currency?.symbol ?? t('Unknown')} ${currency?.network ? `(${currency.network})` : ""}`,Ensure that the localization function
t()is defined and properly configured in your application.
885-895: Improve responsiveness for smaller screens in.searchable-dropdown-container.Currently, the layout adjusts to two columns for screens smaller than 1300px but may still be cramped on mobile devices.
Add a media query for smaller screens:
@media only screen and (max-width: 800px) { .searchable-dropdown-container { grid-template-columns: 1fr; gap: 20px; } }This ensures the dropdowns stack vertically on small screens for better usability.
shared/components/searchable-dropdown.svelte (2)
4-9: Enhance type safety by specifying generic types instead ofany.Using
anyreduces type safety and can introduce bugs. Define a generic type parameter for the component to improve type checking.Adjust the component to use generics:
<script lang="ts"> export let items: T[] = []; export let placeholder = "Search..."; export let onSelect: (item: T) => void; export let getValue: (item: T) => string; export let getDisplayValue?: (item: T) => string; export let disabled = false; let searchTerm = ""; let isOpen = false; let filteredItems: T[] = []; </script>Ensure
getDisplayValueis optional if it's checked in the code.
31-76: Improve accessibility with keyboard navigation and ARIA attributes.The component currently lacks keyboard support and ARIA roles, which are important for accessibility.
Implement keyboard event handlers to allow navigation using arrow keys and selection with the Enter key. Add ARIA roles like
role="combobox"androle="option"to relevant elements to assist screen reader users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
packages/create-invoice-form/src/lib/invoice/form.svelte(6 hunks)shared/components/searchable-dropdown.svelte(1 hunks)shared/icons/network/arbitrum.svelte(1 hunks)shared/icons/network/celo.svelte(1 hunks)shared/icons/network/mantle.svelte(1 hunks)shared/icons/network/matic.svelte(1 hunks)shared/icons/network/network-icon.svelte(1 hunks)shared/icons/network/optimism.svelte(1 hunks)shared/icons/network/ronin.svelte(2 hunks)shared/icons/network/zk-sync.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- shared/icons/network/matic.svelte
- shared/icons/network/mantle.svelte
- shared/icons/network/celo.svelte
- shared/icons/network/optimism.svelte
- shared/icons/network/zk-sync.svelte
- shared/icons/network/arbitrum.svelte
🔇 Additional comments (2)
shared/icons/network/ronin.svelte (1)
15-22: LGTM! Class renaming prevents style conflicts.
The renaming of CSS classes from .st0/.st1 to .st0r/.st1r is a good defensive programming practice to avoid style collisions when multiple network icons are displayed together in the dropdown.
Also applies to: 25-25, 42-42
shared/icons/network/network-icon.svelte (1)
6-16: LGTM!
The addition of the showLabel prop with a default value of true is appropriate and maintains backward compatibility. Conditional rendering works as expected.
…170-allow-type-to-search-for-payment-chain-and-payment-currency-on-create-invoice-form
…170-allow-type-to-search-for-payment-chain-and-payment-currency-on-create-invoice-form
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/create-invoice-form/src/lib/invoice/form.svelte (2)
436-450: Enhance network display with additional metadata.Consider adding network icons or additional metadata (e.g., testnet vs mainnet) to improve network identification.
- label: networkItem[0]?.toUpperCase() + networkItem?.slice(1), + label: `${networkItem[0]?.toUpperCase()}${networkItem?.slice(1)} ${isTestnet(networkItem) ? '(Testnet)' : ''}`,
882-892: Consider enhancing dropdown container accessibility.While the responsive grid layout works well, consider adding ARIA attributes and keyboard navigation support for better accessibility.
.searchable-dropdown-container { display: grid; grid-template-columns: repeat(3, minmax(0, 1fr)); gap: 30px; + role: "group"; + aria-label: "Payment Options"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/create-invoice-form/src/lib/invoice/form.svelte(6 hunks)
🔇 Additional comments (4)
packages/create-invoice-form/src/lib/invoice/form.svelte (4)
8-8: LGTM: Component import is properly organized.
The SearchableDropdown import is correctly placed with other component imports.
571-589: Date input implementation looks good.
The date formatting and validation have been implemented correctly as suggested in the previous review.
451-461: Verify settlement currency filtering logic.
The settlement currency options depend on the filtered currencies from a reactive statement. Ensure the filtering logic handles all edge cases.
✅ Verification successful
Let me gather more information about the settlement currency filtering logic.
Let me gather one more piece of information about the default currencies and currency types.
Based on the gathered information, I can now provide the final response:
Settlement currency filtering logic appears to be properly implemented
The filtering logic handles the following cases:
- Filters out currencies when no invoice currency is selected
- Has special handling for ISO4217 currencies (like EUR)
- Validates conversion paths using
currencyManager.getConversionPath - Filters currencies based on the selected network
- Uses the currency manager's known currencies as the source
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for currency conversion path implementations
rg -A 5 "getConversionPath"
Length of output: 1061
Script:
#!/bin/bash
# Search for filteredSettlementCurrencies implementation and related code
rg -A 10 "filteredSettlementCurrencies"
# Search for currency filtering related reactive statements
ast-grep --pattern '$: {
$$$
}' packages/create-invoice-form/src/lib/invoice/form.svelte
Length of output: 3237
Script:
#!/bin/bash
# Search for defaultCurrencies initialization and currency type definitions
rg -A 5 "defaultCurrencies ="
# Search for currency type definitions
rg "CURRENCY\s*=\s*{" -A 5
Length of output: 1744
416-435: Consider adding error handling for undefined currency symbols.
The currency display logic assumes symbols will always be available. Add a fallback for cases where the symbol might be undefined.
- getValue={(currency) => currency.value.symbol}
+ getValue={(currency) => currency.value?.symbol ?? 'Unknown'}…nd-payment-currency-on-create-invoice-form
…170-allow-type-to-search-for-payment-chain-and-payment-currency-on-create-invoice-form
…currency-on-create-invoice-form' of github.com:RequestNetwork/web-components into 170-allow-type-to-search-for-payment-chain-and-payment-currency-on-create-invoice-form
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
shared/components/searchable-dropdown.svelte (2)
4-9: Consider adding TypeScript interfaces for props.The component uses generic
anytypes which could lead to runtime errors. Consider defining proper interfaces for the items and callback functions.+interface DropdownItem { + value: any; + label?: string; + type?: string; +} -export let items: any[] = []; +export let items: DropdownItem[] = []; -export let onSelect: (item: any) => void; +export let onSelect: (value: any) => void; -export let getValue: (item: any) => string; +export let getValue: (item: DropdownItem) => string; -export let getDisplayValue: (item: any) => string; +export let getDisplayValue: (item: DropdownItem) => string;
134-141: Consider making the input width configurable.The input width is hardcoded to 75%, which might not be suitable for all use cases.
+export let inputWidth = "75%"; .searchable-dropdown input { position: relative; padding: 8px 12px 8px 36px; border: 1px solid #ccc; border-radius: 4px; font-size: 14px; - width: 75%; + width: var(--input-width, inputWidth); }packages/create-invoice-form/src/lib/invoice/form.svelte (1)
883-887: Improve responsive design for mobile devices.The grid layout might not work well on smaller screens. Consider adding more breakpoints.
.searchable-dropdown-container { display: grid; grid-template-columns: repeat(3, minmax(0, 1fr)); gap: 30px; } +@media only screen and (max-width: 768px) { + .searchable-dropdown-container { + grid-template-columns: 1fr; + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/create-invoice-form/src/lib/create-invoice-form.svelte(4 hunks)packages/create-invoice-form/src/lib/invoice/form-view.svelte(1 hunks)packages/create-invoice-form/src/lib/invoice/form.svelte(7 hunks)shared/components/searchable-dropdown.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/create-invoice-form/src/lib/invoice/form-view.svelte
🔇 Additional comments (2)
packages/create-invoice-form/src/lib/create-invoice-form.svelte (1)
118-118: Verify the impact of initializing cipherProvider as undefined.
The change in cipherProvider initialization could affect encryption functionality. Ensure that encryption still works as expected when required.
✅ Verification successful
The initialization of cipherProvider as undefined is safe and intentional
The codebase handles the undefined cipherProvider gracefully:
- In create-invoice-form.svelte, encryption is only attempted when both cipherProvider exists and formData.isEncrypted is true
- The encryption checkbox in the form is only shown when cipherProvider is available
- This change aligns with the pattern in view-requests.svelte where cipherProvider is optionally derived from requestNetwork
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for places where cipherProvider is used
rg -A 5 "cipherProvider" --type svelte
Length of output: 69
Script:
#!/bin/bash
# Search for places where cipherProvider is used with regular ripgrep
rg -A 5 "cipherProvider"
Length of output: 6430
packages/create-invoice-form/src/lib/invoice/form.svelte (1)
584-602: Date input implementation matches previous review.
The date input implementation follows the suggested fix from the previous review.
sstefdev
deleted the
170-allow-type-to-search-for-payment-chain-and-payment-currency-on-create-invoice-form
branch
Fixes #170
Fixes #222 - Move Issued and Due Date fields inline with the rest of the form
Fixes Partially (Quantity input size) - #1528
Problem
Scrolling through the Payment Chain and Payment Currency dropdowns to find specific options is cumbersome and inefficient for users.
Changes:
Summary by CodeRabbit
Release Notes
New Features
SearchableDropdowncomponent for selecting invoice currency and payment chain, enhancing user experience with searchable options.showLabelto the network icon component, allowing conditional rendering of the network name.Bug Fixes
Style