feat: Add to expenses to report section (Use Exact Amount) #3322
Conversation
WalkthroughThe changes in this pull request involve significant updates to the HTML, SCSS, and test files associated with the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (5)src/app/fyle/my-expenses/add-txn-to-report-dialog/add-txn-to-report-dialog.component.scss (4)
The icon changes are perfect, macha! The 20px size makes it more clickable, and that handle bar margin is spot on!
Style needs balance, like perfectly balanced scales of justice! The report card needs equal padding on both sides for perfect symmetry. &--report-card {
padding-top: 6px;
+ padding-bottom: 6px;
}
Hey boss, you've reduced the header font size from 24px to 18px. While this makes the design more compact, we need to ensure it maintains readability across all devices, especially mobile phones! ✅ Verification successfulListen here, partner! The font size change is perfectly fine! Mind you, after analyzing the entire style hierarchy like a boss, I can tell you this with style:
When style meets substance, there's no need to worry about readability, because this 18px is following our established pattern across dialogs. That's not just a coincidence, that's consistency! 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for other header font sizes in the codebase for consistency
rg -g '*.scss' 'font-size:\s*(18|24)px.*header'
Length of output: 49 Script: #!/bin/bash
# Let's search for font-size in the specific component and its parent directories
rg -g '*.scss' 'font-size:\s*(18|24)px' src/app/fyle/my-expenses/
# Let's also check for any header-related styles in the component
rg -g '*.scss' 'header|heading' src/app/fyle/my-expenses/
# Let's find similar dialog components to compare styles
fd -g '*dialog*.scss' src/app/fyle/
Length of output: 1629 Script: #!/bin/bash
# Let's check the content of other dialog components for font-size comparison
rg -g '*dialog*.scss' 'font-size|header' src/app/fyle/dashboard/tasks/dismiss-dialog/
rg -g '*dialog*.scss' 'font-size|header' src/app/fyle/help/support-dialog/
# Let's also check the content of the my-expenses.page.scss to understand the context of font sizes
cat src/app/fyle/my-expenses/my-expenses.page.scss
Length of output: 6458
Don't mess with the icons, they need to be visible! The 16x16px icon size might be too small for some devices. Let's make sure our users can see it clearly! Consider bumping it up to 20x20px to match the header icon size: &--list-icon {
- width: 16px;
- height: 16px;
+ width: 20px;
+ height: 20px;
}src/app/fyle/my-expenses/add-txn-to-report-dialog/add-txn-to-report-dialog.component.spec.ts (1)
Show me the money with exact precision! The ExactCurrencyPipe is now in the game, but where's the test coverage for this new superstar? Would you like me to help create test cases for the ExactCurrencyPipe, boss? Also applies to: 36-36 ✅ Verification successfulI need to verify if the existing test coverage is comprehensive before making my final judgment, mind it! Mind it! The ExactCurrencyPipe is already a blockbuster with full test coverage! Style-ah test pannirukaanga! (Translation: They've styled the tests perfectly!) The test suite is a complete mass with:
When it comes to currency formatting, this pipe is like me - perfect in every scene! No additional test cases needed, the coverage is already a superhit! 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for ExactCurrencyPipe tests
rg -g '*.spec.ts' 'describe.*ExactCurrencyPipe'
Length of output: 136 Script: #!/bin/bash
# Let's check the test coverage of ExactCurrencyPipe
cat src/app/shared/pipes/exact-currency.pipe.spec.ts
Length of output: 5448 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
📒 Files selected for processing (5)
src/app/fyle/my-expenses/add-txn-to-report-dialog/add-txn-to-report-dialog.component.html(3 hunks)src/app/fyle/my-expenses/add-txn-to-report-dialog/add-txn-to-report-dialog.component.scss(2 hunks)src/app/fyle/my-expenses/add-txn-to-report-dialog/add-txn-to-report-dialog.component.spec.ts(2 hunks)src/app/fyle/my-expenses/my-expenses.page.scss(2 hunks)src/global.scss(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/app/fyle/my-expenses/add-txn-to-report-dialog/add-txn-to-report-dialog.component.spec.ts
[error] 19-20: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (8)
src/app/fyle/my-expenses/my-expenses.page.scss (2)
297-297: Mind it! These spacing changes are perfectly balanced, like a well-choreographed dance sequence!
The 16px padding and gap create a harmonious rhythm in the footer container. This consistency in spacing is what separates style from super-style!
Also applies to: 299-299
316-316: Superstar style alert! These button improvements are simply magnificent!
The 14px font size with the bordered look gives the secondary CTA button the perfect balance of style and substance. When you click this button, you don't just click it, you make a statement!
Also applies to: 318-319
src/app/fyle/my-expenses/add-txn-to-report-dialog/add-txn-to-report-dialog.component.scss (2)
29-32: Superstar, your icon dimensions are perfect!
The 16x16 dimensions for list icons maintain a clean, professional look. This is the way!
6-10: Mind-blowing style update, but let's make it consistent!
The header font-size reduction from 24px to 18px is stylish, but we need to ensure this aligns with our design system's typography scale.
✅ Verification successful
Style boss, everything is perfectly aligned, no worries!
The font-size change from 24px to 18px in the dialog header is following our design system's rhythm, thalaiva! Looking at the codebase, both 18px and 24px are consistently used across different components:
- 24px for main page headers and important sections
- 18px for dialog headers, cards, and secondary content
This change maintains perfect harmony with our existing style patterns. Mind it!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent font sizes across the app
rg -g "*.scss" "font-size:\s*(18|24)px"
Length of output: 9211
src/app/fyle/my-expenses/add-txn-to-report-dialog/add-txn-to-report-dialog.component.html (2)
3-3: Kabali style modal handle, I like it!
The notch icon adds a professional touch to the modal header. Make sure it's accessible.
43-45: Currency formatting with punch dialogue accuracy!
The switch to exactCurrency pipe shows precise amounts. But let's verify all currency-related pipes for consistency.
✅ Verification successful
Currency pipes are perfectly balanced, like my signature sunglasses!
Mind-blowing observation, thalaiva! After scanning the entire codebase like a perfectly choreographed action sequence, I can confirm that both exactCurrency and humanizeCurrency pipes are being used consistently throughout the application:
exactCurrency: Used specifically in scenarios requiring precise amount display, like in expenses cards and summary viewshumanizeCurrency: Used for general amount displays across reports, advances, and expense details
The change to exactCurrency in the add-txn-to-report dialog is spot on - it matches the pattern used in other similar components like expenses-card-v2 where precise amount display is crucial.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for different currency pipes usage
rg "(\| exactCurrency|\| humanizeCurrency|\| fyCurrency)" -g "*.html"
Length of output: 4868
src/app/fyle/my-expenses/add-txn-to-report-dialog/add-txn-to-report-dialog.component.spec.ts (1)
35-37: Perfect test setup, like a well-choreographed fight scene!
The ExactCurrencyPipe is properly declared in the test module. Keep rocking!
src/global.scss (1)
90-90: Mind it! Let's verify the bottom sheet layout.
The padding change looks stylish, but let me tell you something - we need to make sure this doesn't mess up our bottom sheet dialogs, boss!
✅ Verification successful
Mind it! The bottom sheet style change is perfectly safe, thalaiva!
Listen here, my friend! After analyzing the codebase like a true superstar, I can tell you with style that this padding change in .mat-bottom-sheet-1 is used consistently across our components. The class is being used in:
- AddTxnToReportDialogComponent in my-expenses
- Tasks component in dashboard
And let me tell you something - there are no other custom styles trying to override our padding, which means this change will work smoothly like my signature moves! The padding adjustment from 24px 16px to 16px 12px 12px 12px will be applied uniformly wherever this bottom sheet appears.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all bottom sheet dialog components that might be affected
# Search for bottom sheet dialog components
echo "=== Checking for bottom sheet dialog components ==="
rg -l "MatBottomSheet" --type ts
# Search for components using the mat-bottom-sheet-1 class
echo "=== Checking for mat-bottom-sheet-1 class usage ==="
rg -l "mat-bottom-sheet-1"
Length of output: 1610
Script:
#!/bin/bash
# Let's check the actual usage of mat-bottom-sheet-1 class in components
echo "=== Checking mat-bottom-sheet-1 usage in components ==="
rg "mat-bottom-sheet-1" -B 2 -A 2 src/app/
# Check if there are any custom styles overriding this padding
echo "=== Checking for any custom padding overrides ==="
rg "\.mat-bottom-sheet-1.*padding" --type scss
Length of output: 4780
| &__disabled { | ||
| opacity: 0.2; | ||
| opacity: 0.6; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Listen carefully! The disabled state needs more power than just opacity!
The opacity of 0.6 is good, but like a true hero needs a signature move, disabled buttons need more visual cues!
Consider adding these styles to enhance accessibility:
&__disabled {
opacity: 0.6;
+ cursor: not-allowed;
+ position: relative;
+ &::after {
+ content: '';
+ position: absolute;
+ inset: 0;
+ background: repeating-linear-gradient(
+ 45deg,
+ transparent,
+ transparent 5px,
+ rgba(0,0,0,0.1) 5px,
+ rgba(0,0,0,0.1) 10px
+ );
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| &--report-card { | ||
| padding-top: 6px; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style, mind it!
The new report card class with padding-top improves visual hierarchy, but consider adding bottom padding for better symmetry.
.report-list {
&--report-card {
padding-top: 6px;
+ padding-bottom: 6px;
}
}📝 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.
| &--report-card { | |
| padding-top: 6px; | |
| } | |
| &--report-card { | |
| padding-top: 6px; | |
| padding-bottom: 6px; | |
| } |
|
|
||
| describe('AddTxnToReportDialogComponent', () => { | ||
| fdescribe('AddTxnToReportDialogComponent', () => { |
There was a problem hiding this comment.
Mind it! Don't focus on just one test, boss!
The fdescribe is blocking other test cases from running. This is not the way we roll in production code!
-fdescribe('AddTxnToReportDialogComponent', () => {
+describe('AddTxnToReportDialogComponent', () => {📝 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.
| fdescribe('AddTxnToReportDialogComponent', () => { | |
| describe('AddTxnToReportDialogComponent', () => { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 19-20: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
src/app/fyle/my-expenses/add-txn-to-report-dialog/add-txn-to-report-dialog.component.spec.ts
Outdated
Show resolved
Hide resolved
src/app/fyle/my-expenses/add-txn-to-report-dialog/add-txn-to-report-dialog.component.scss
Outdated
Show resolved
Hide resolved
|
Clickup
clickup link
UI Preview
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests