feat: View advance request display exact amount (SPENDER + APPROVER) #3353
Conversation
WalkthroughThe changes in this pull request involve modifications to the HTML and SCSS files of the Changes
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/app/shared/components/summary-tile/summary-tile.component.html(2 hunks)src/app/shared/components/summary-tile/summary-tile.component.scss(2 hunks)src/app/shared/components/summary-tile/summary-tile.component.spec.ts(2 hunks)
🔇 Additional comments (3)
src/app/shared/components/summary-tile/summary-tile.component.html (1)
19-20: Magizhchi! The exactCurrency pipe implementation looks sharp!
The new currency formatting with exactCurrency pipe is a style upgrade that shows precise amounts. But like my punch dialogues, we need proper test coverage!
✅ Verification successful
Mind it! Your ExactCurrencyPipe is fully loaded with test coverage, just like my action sequences!
The test coverage is as perfect as my signature sunglasses flip! You've got comprehensive test cases covering:
- Positive and negative values
- Currency symbol handling
- Custom fraction digits
- Zero value handling
- Large number formatting
- All configuration combinations
The implementation matches perfectly with the test cases, like a well-choreographed fight scene. No gaps in coverage found!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if ExactCurrencyPipe has proper test coverage
rg -l "describe.*ExactCurrencyPipe"
Length of output: 86
Script:
#!/bin/bash
# Let's check the actual test coverage of ExactCurrencyPipe
cat src/app/shared/pipes/exact-currency.pipe.spec.ts
Length of output: 5448
Script:
#!/bin/bash
# Let's check the implementation to ensure all cases are covered
cat src/app/shared/pipes/exact-currency.pipe.ts
Length of output: 887
src/app/shared/components/summary-tile/summary-tile.component.scss (1)
22-22: 🧹 Nitpick (assertive)
Style pandratha nalla pannu! (Do styling properly!)
The font size reductions from 18px to 14px and color changes to dark-grey might affect readability. Like my movies' cinematography, every detail matters!
Let's ensure these changes align with design system:
Consider using CSS variables for consistent styling:
- font-size: 14px;
+ font-size: var(--font-size-regular);
- color: $dark-grey;
+ color: var(--color-text-secondary);Also applies to: 28-28, 34-34, 35-35, 39-39, 40-40
✅ Verification successful
Style is all about consistency, and this change is perfectly in sync with our shared components!
The font-size reduction to 14px is actually following a well-established pattern across our shared components. The ripgrep results show that 14px is widely used across more than 50 shared components, making it a de-facto standard in our design system. Just like how I maintain my signature style across all my movies, this font size maintains consistency across our UI!
While the suggestion to use CSS variables is stylish, the current implementation is already following the established pattern in our codebase. The readability concerns are unwarranted since this font size is proven to work well across numerous components.
Remember what I always say: "When something works in style, don't fix it!"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar font sizes in other components
rg -l "font-size:\s*(12|14)px" "src/app/shared/components"
Length of output: 5321
src/app/shared/components/summary-tile/summary-tile.component.spec.ts (1)
19-26:
Test cases are like my stunts - they must be perfect!
The ExactCurrencyPipe is added to declarations, but like my movie's continuity, we need to update the test selectors!
Update the test selectors to match new class names:
- '.summary-tile--card-container__category'
+ '.summary-title--card-container__category'And similar changes throughout the test file.
Likely invalid or redundant comment.
src/app/shared/components/summary-tile/summary-tile.component.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/app/shared/components/summary-tile/summary-tile.component.html(2 hunks)src/app/shared/components/summary-tile/summary-tile.component.scss(1 hunks)
🔇 Additional comments (4)
src/app/shared/components/summary-tile/summary-tile.component.html (2)
1-1: Mind it! The class naming needs attention!
Hey boss, the class names are changed from 'summary-tile' to 'summary-title', but the component is still named 'summary-tile'. This inconsistency will confuse other developers like a plot twist in my movies!
20-20: Mind it! The currency pipe change looks powerful!
The switch from humanizeCurrency to exactCurrency pipe is exactly what we need for precise amount display. But like in my movies, we need to verify all the scenes!
✅ Verification successful
Mind it! The exactCurrency pipe is perfectly set up, like a well-choreographed action sequence!
The pipe is properly declared in SharedModule and its implementation is rock solid! It handles currency formatting with style, maintaining exact precision while respecting currency codes and symbols. When I make a style statement, I make it perfect - and this pipe does exactly that!
- Properly imported and exported in SharedModule ✨
- Handles negative numbers with finesse 💪
- Maintains exact decimal precision 🎯
- Works in perfect sync with FyCurrencyPipe 🌟
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if exactCurrency pipe is properly imported and declared
ast-grep --pattern 'import { ExactCurrencyPipe } from $_'
Length of output: 59
Script:
#!/bin/bash
# Let's search for the ExactCurrencyPipe class definition and its module declaration
ast-grep --pattern 'export class ExactCurrencyPipe implements PipeTransform'
# Also search for the pipe declaration in any module
ast-grep --pattern '@NgModule({
$$$
declarations: [$$$ExactCurrencyPipe$$$]
$$$
})'
# And let's check where this pipe is actually used
rg "exactCurrency" -A 2
Length of output: 5149
Script:
#!/bin/bash
# Let's check the shared module for the pipe declaration, mind it!
rg -l "SharedModule" | xargs rg "ExactCurrencyPipe"
# And check the implementation of our powerful pipe, thalaiva style!
cat src/app/shared/pipes/exact-currency.pipe.ts
Length of output: 1144
src/app/shared/components/summary-tile/summary-tile.component.scss (2)
35-35: Superstar! These colors are perfect!
The color changes to $dark-grey and $black improve contrast like the perfect lighting in my movie scenes!
Also applies to: 40-40, 45-45
50-51: Kabali da! New amount container looks stylish!
The new amount container with margin-top: 10px gives better spacing. But like my movie's timing, we need to ensure it's responsive!
|
Clickup
clickup link
UI Preview
Please add screenshots for UI changes
Summary by CodeRabbit
New Features
Style
Tests
ExactCurrencyPipeto the test suite for enhanced testing capabilities.