-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
fix: denom info modal #37
Conversation
WalkthroughThe changes in the Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🪛 GitHub Check: codecov/patchcomponents/factory/modals/denomInfo.tsx[warning] 14-14: components/factory/modals/denomInfo.tsx#L14 [warning] 71-71: components/factory/modals/denomInfo.tsx#L71 [warning] 79-79: components/factory/modals/denomInfo.tsx#L79 🔇 Additional comments (5)components/factory/modals/denomInfo.tsx (4)
The new condition for detecting factory addresses lacks test coverage. Additionally, consider making the prefix check more robust. - if (denom?.name.startsWith('factory/manifest1')) {
+ const FACTORY_PREFIX = 'factory/manifest1';
+ if (denom?.name?.toLowerCase().startsWith(FACTORY_PREFIX)) { Let me help you write test cases for this logic. Would you like me to generate them? 🧰 Tools🪛 GitHub Check: codecov/patch[warning] 14-14: components/factory/modals/denomInfo.tsx#L14
The changes perfectly align with the PR objectives, providing a cleaner interface with just the essential information. Good use of fallback values and appropriate text transformations.
The addition of the optional className prop and full-height styling enhances the component's flexibility and consistency.
The external link implementation and address truncation logic lack test coverage. This should be tested to ensure proper functionality across different address formats. Run this script to check existing test coverage for similar components: Would you like me to help generate test cases for:
🧰 Tools🪛 GitHub Check: codecov/patch[warning] 71-71: components/factory/modals/denomInfo.tsx#L71 [warning] 79-79: components/factory/modals/denomInfo.tsx#L79 components/bank/components/__tests__/tokenList.test.tsx (1)
The test assertions have been properly updated to match the simplified modal design showing only Name, Ticker, and Description fields. Let's verify if there are any remaining test assertions for removed fields that need cleanup: ✅ Verification successfulTest assertions are properly cleaned up, no remaining references to removed fields After reviewing the complete test file, I can confirm that all test assertions have been properly updated. The modal test now only checks for the simplified fields (Name, Ticker, Description) and there are no lingering assertions for removed fields like EXPONENT or DENOM_UNITS. While these fields exist in the mock data structure, they are not being tested in any assertions, which is correct. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Search for any remaining test assertions related to removed fields
# like EXPONENT, DENOM, or denom_units in test files
rg -i "expect\(.*?(exponent|denom_units)" --type ts,tsx "components/bank"
Length of output: 104 Script: #!/bin/bash
# Search for any remaining test assertions related to removed fields
# Correcting the file type flags and expanding the search patterns
rg -t tsx -t ts "expect.*?(EXPONENT|exponent|denom_units|DENOM_UNITS)" components/bank/
Length of output: 117 Script: #!/bin/bash
# Search for any remaining test assertions related to removed fields
# Using simpler ripgrep syntax without file type flags
rg "expect.*?(EXPONENT|exponent|denom_units|DENOM_UNITS)" "components/bank/__tests__" "components/bank/components/__tests__"
Length of output: 188 Script: #!/bin/bash
# First, let's find the correct test file locations
fd "test.tsx" components/bank/
# Then search for any remaining test assertions related to removed fields
# Using a broader search to find any test expectations
rg "expect.*?(EXPONENT|exponent|denom|DENOM)" $(fd "test.tsx" components/bank/)
Length of output: 650 Script: #!/bin/bash
# Let's examine the content of the tokenList test file to see all expectations
cat components/bank/components/__tests__/tokenList.test.tsx
Length of output: 3596 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
==========================================
- Coverage 59.39% 59.24% -0.15%
==========================================
Files 148 148
Lines 13830 13811 -19
==========================================
- Hits 8214 8183 -31
- Misses 5616 5628 +12 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
This PR reworks the UI of the denom info modal to keep only the token name, ticker, and description.
Fixes #29
Summary by CodeRabbit
New Features
className
prop in the InfoItem component.Bug Fixes