Better formating and style for invoice info #202
Conversation
WalkthroughThe pull request introduces modifications to the 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
packages/create-invoice-form/src/lib/invoice/form-view.svelte (4)
51-93: Add error handling for malformed address objectsWhile the address formatting logic is clean, it could benefit from defensive programming to handle malformed address objects.
Consider adding null checks:
const addressParts = []; -if (info.address["street-address"]) +if (info.address?.["street-address"]) addressParts.push(info.address["street-address"]); -if (info.address["locality"]) addressParts.push(info.address["locality"]); +if (info.address?.["locality"]) addressParts.push(info.address["locality"]);
162-172: Consider extracting common rendering logicThe buyer and seller information sections share identical markup. Consider extracting this into a reusable component.
Example component structure:
<!-- ContactInfo.svelte --> <script lang="ts"> export let details: Array<{label?: string, value: string, isEmail?: boolean, isCompany?: boolean}>; </script> <div class="invoice-info"> {#each details as { value, isEmail, isCompany }} <p> {#if isEmail} <a href="mailto:{value}">{value}</a> {:else} <span class:company={isCompany}>{value}</span> {/if} </p> {/each} </div>
501-532: Add focus styles for better accessibilityWhile the hover styles are good, adding focus styles would improve accessibility for keyboard users.
Add focus styles to the links:
.invoice-info a:hover { text-decoration: underline; } +.invoice-info a:focus { + text-decoration: underline; + outline: 2px solid var(--mainColor); + outline-offset: 2px; +}
176-179: Simplify network name capitalizationThe current string manipulation for capitalizing the network name could be simplified using CSS.
-{currency?.network - ? currency.network.charAt(0).toUpperCase() + - currency.network.slice(1).toLowerCase() - : ""} +<span class="network-name">{currency?.network || ""}</span> +<style> +.network-name { + text-transform: capitalize; +} +</style>packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
34-38: Fix typo in interface nameThe interface name contains a spelling error:
BuyerSelerInfoshould beBuyerSellerInfo.-interface BuyerSelerInfo { +interface BuyerSellerInfo { value: string; isCompany?: boolean; isEmail?: boolean; }
405-414: Consider extracting duplicated rendering logic into a reusable componentThe rendering logic for seller and buyer information is identical. Consider creating a reusable component to reduce code duplication and improve maintainability.
Example implementation:
<!-- ContactInfo.svelte --> <script lang="ts"> export let info: BuyerSellerInfo[]; </script> <div class="invoice-info"> {#each info as { value, isCompany, isEmail }} <p> {#if isEmail} <a href="mailto:{value}" class="email-link">{value}</a> {:else if isCompany} <span class="company-name">{value}</span> {:else} {value} {/if} </p> {/each} </div>Usage:
-<div class={`invoice-info`}> - {#each sellerInfo as { value, isCompany, isEmail }} - <p> - {#if isEmail} - <a href="mailto:{value}" class="email-link">{value}</a> - {:else if isCompany} - <span class="company-name">{value}</span> - {:else} - {value} - {/if} - </p> - {/each} -</div> +<ContactInfo info={sellerInfo} />Also applies to: 425-434
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/create-invoice-form/src/lib/invoice/form-view.svelte(3 hunks)packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(8 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/create-invoice-form/src/lib/invoice/form-view.svelte (1)
Learnt from: MantisClone
PR: RequestNetwork/web-components#155
File: packages/create-invoice-form/src/lib/invoice/form-view.svelte:191-194
Timestamp: 2024-11-12T14:52:33.204Z
Learning: When aligning with the pre-existing `rnf invoice 0.0.3` format in `packages/create-invoice-form/src/lib/invoice/form-view.svelte`, adding comments to explain changes from `description` to `name` is unnecessary.
🔇 Additional comments (4)
packages/create-invoice-form/src/lib/invoice/form-view.svelte (2)
144-155: LGTM! Clean and semantic markup
The seller information section is well-structured with appropriate conditional rendering and semantic HTML elements.
156-156: LGTM! Good visual separation
The section divider provides clear visual separation while maintaining design consistency by using the mainColor variable.
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (2)
74-104: LGTM! Well-structured detail generation logic
The implementation effectively:
- Combines name fields with proper null checks
- Formats addresses with appropriate separators
- Handles special cases for company names and emails
686-690: LGTM! Well-structured styles
The new styles effectively:
- Enhance visual hierarchy with proper spacing and colors
- Use semantic color variables
- Provide clear interactive states for email links
Also applies to: 903-913
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte
Outdated
Show resolved
Hide resolved
…165-remove-postal-address-labels-from-invoice-view
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
packages/create-invoice-form/src/lib/invoice/form-view.svelte (3)
144-170: Consider extracting repeated entity info template into a componentThe seller and buyer sections share identical template structure. Following DRY principles, this could be extracted into a reusable component.
Consider creating a new component like
EntityInfoDisplay.svelte:<!-- EntityInfoDisplay.svelte --> <script lang="ts"> export let entityInfo: Array<{value: string, isEmail?: boolean, isCompany?: boolean}>; </script> <div class="invoice-info"> {#each entityInfo as { value, isEmail, isCompany }} <p> {#if isEmail} <a href="mailto:{value}">{value}</a> {:else} <span class:company={isCompany}>{value}</span> {/if} </p> {/each} </div>Then use it in both sections:
- <div class={`invoice-info`}> - {#each sellerInfo as { value, isEmail, isCompany }} - <p> - {#if isEmail} - <a href="mailto:{value}">{value}</a> - {:else} - <span class:company={isCompany}>{value}</span> - {/if} - </p> - {/each} - </div> + <EntityInfoDisplay entityInfo={sellerInfo} />
501-532: Consider using CSS variables for colors and spacingThe new styles are well-structured, but could benefit from using CSS variables for better maintainability and consistency.
Consider this improvement:
+ :root { + --text-muted: #6e7480; + --spacing-xs: 6px; + --spacing-sm: 8px; + } .invoice-info { display: flex; flex-direction: column; - gap: 6px; + gap: var(--spacing-xs); } .invoice-info p { display: flex; - gap: 8px; + gap: var(--spacing-sm); } .invoice-info span { - color: #6e7480; + color: var(--text-muted); } .invoice-info a { - color: #6e7480; + color: var(--text-muted); }
176-179: Simplify network name capitalizationThe current string manipulation for capitalizing the network name could be simplified.
Consider this improvement:
- {currency?.network - ? currency.network.charAt(0).toUpperCase() + - currency.network.slice(1).toLowerCase() - : ""} + {currency?.network?.toLowerCase()?.replace(/^\w/, c => c.toUpperCase()) ?? ""}Or if this transformation is used elsewhere, consider extracting it into a utility function:
const capitalizeFirst = (str: string) => str.toLowerCase().replace(/^\w/, c => c.toUpperCase());packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (3)
77-102: Consider adding input validation for the address object.While the address formatting logic is solid, it might be good to add validation for the address object structure to prevent potential runtime errors if the object is malformed.
const generateDetailParagraphs = (info: any) => { + // Validate address object structure + const hasValidAddress = info?.address && typeof info.address === 'object'; + const fullName = [info?.firstName, info?.lastName] .filter(Boolean) .join(" "); const fullAddress = [ - info?.address?.["street-address"], - info?.address?.locality, - info?.address?.region, - info?.address?.["postal-code"], - info?.address?.["country-name"], + hasValidAddress ? info.address["street-address"] : null, + hasValidAddress ? info.address.locality : null, + hasValidAddress ? info.address.region : null, + hasValidAddress ? info.address["postal-code"] : null, + hasValidAddress ? info.address["country-name"] : null, ] .filter(Boolean) .join(", ");
402-411: Consider extracting a reusable component for entity information.The seller and buyer information sections share identical rendering logic. Consider extracting this into a reusable component to reduce code duplication and improve maintainability.
<!-- EntityInfo.svelte --> <script lang="ts"> export let info: Array<{ value: string; isCompany?: boolean; isEmail?: boolean }>; </script> <div class="invoice-info"> {#each info as { value, isCompany, isEmail }} <p> {#if isEmail} <a href="mailto:{value}" class="email-link">{value}</a> {:else if isCompany} <span class="company-name">{value}</span> {:else} {value} {/if} </p> {/each} </div>Usage:
-<div class={`invoice-info`}> - {#each sellerInfo as { value, isCompany, isEmail }} - <p> - {#if isEmail} - <a href="mailto:{value}" class="email-link">{value}</a> - {:else if isCompany} - <span class="company-name">{value}</span> - {:else} - {value} - {/if} - </p> - {/each} -</div> +<EntityInfo info={sellerInfo} />Also applies to: 422-431
683-687: Consider using CSS variables for colors.The styling changes look good, but consider using CSS variables for colors to maintain consistency and make theme changes easier.
.invoice-info { display: flex; flex-direction: column; gap: 6px; width: fit-content; - color: #6e7480; + color: var(--text-secondary); font-size: 16px; } .company-name { font-weight: 600 !important; } .email-link { - color: #6e7480; + color: var(--text-secondary); }Also applies to: 900-910
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/create-invoice-form/src/lib/invoice/form-view.svelte(3 hunks)packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte(7 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/create-invoice-form/src/lib/invoice/form-view.svelte (1)
Learnt from: MantisClone
PR: RequestNetwork/web-components#155
File: packages/create-invoice-form/src/lib/invoice/form-view.svelte:191-194
Timestamp: 2024-11-12T14:52:33.204Z
Learning: When aligning with the pre-existing `rnf invoice 0.0.3` format in `packages/create-invoice-form/src/lib/invoice/form-view.svelte`, adding comments to explain changes from `description` to `name` is unnecessary.
🔇 Additional comments (1)
packages/invoice-dashboard/src/lib/dashboard/invoice-view.svelte (1)
34-42: LGTM! Well-structured interface definitions.
The new interfaces provide a clear and type-safe structure for handling entity information, and the implementation matches the agreed-upon approach from the previous review discussion.
Also applies to: 65-66
sstefdev
deleted the
165-remove-postal-address-labels-from-invoice-view
branch
Fixes #165
Problem
Postal address labels occupy excessive space and are redundant since the structure of the address makes its purpose clear to the end-user.
Changes
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Style