fix: asset list modal opens 1 click #116
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #116 +/- ##
==========================================
- Coverage 60.00% 60.00% -0.01%
==========================================
Files 146 146
Lines 14485 14484 -1
==========================================
- Hits 8692 8691 -1
Misses 5793 5793 ☔ View full report in Codecov by Sentry. |
WalkthroughThe changes in this pull request involve modifying the rendering logic of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (2)
components/bank/components/tokenList.tsx (2)
110-113: LGTM! The unconditional modal rendering fixes the issue.While this change correctly ensures the modal element exists before click handlers try to access it, we could improve the implementation further.
Consider managing the modal's visibility through React state instead of direct DOM manipulation:
- const [selectedDenom, setSelectedDenom] = useState<any>(null); + const [selectedDenom, setSelectedDenom] = useState<string | null>(null); + const [isModalOpen, setIsModalOpen] = useState(false); // In the click handlers, replace showModal() with state: - (document?.getElementById(`denom-info-modal`) as HTMLDialogElement)?.showModal(); + setIsModalOpen(true); // Update modal component: <DenomInfoModal denom={filteredBalances.find(b => b.denom === selectedDenom)?.metadata ?? null} - modalId="denom-info-modal" + isOpen={isModalOpen} + onClose={() => setIsModalOpen(false)} />This approach:
- Uses React state to control modal visibility
- Eliminates direct DOM manipulation
- Improves type safety with proper typing for selectedDenom
Line range hint
40-47: Consolidate duplicate modal trigger logicThe modal opening logic is duplicated between the row click and arrow button click handlers. This could lead to maintenance issues.
Consider extracting the shared logic into a reusable function:
+ const handleOpenModal = (denom: string, event?: React.MouseEvent) => { + event?.stopPropagation(); + setSelectedDenom(denom); + setIsModalOpen(true); + }; // In the row div: - onClick={() => { - setSelectedDenom(balance?.denom); - (document?.getElementById(`denom-info-modal`) as HTMLDialogElement)?.showModal(); - }} + onClick={() => handleOpenModal(balance.denom)} // In the arrow button: - onClick={e => { - e.stopPropagation(); - setSelectedDenom(balance?.denom); - (document?.getElementById(`denom-info-modal`) as HTMLDialogElement)?.showModal(); - }} + onClick={e => handleOpenModal(balance.denom, e)}This refactor:
- Eliminates code duplication
- Centralizes modal opening logic
- Makes the code more maintainable
Also applies to: 89-97
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/bank/components/tokenList.tsx(1 hunks)
🔇 Additional comments (1)
components/bank/components/tokenList.tsx (1)
110-113: Verify modal component interface and usage pattern
Let's verify the DenomInfoModal component interface and check for similar patterns across the codebase.
✅ Verification successful
Props interface matches the implementation
The DenomInfoModal component accepts exactly the props being passed:
denom: acceptsMetadataSDKType | nulltypemodalId: accepts string type
The usage in tokenList.tsx is consistent with both the component's interface and the existing usage pattern in MyDenoms.tsx.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check DenomInfoModal implementation and usage
echo "Checking DenomInfoModal implementation:"
ast-grep --pattern 'export $_ function DenomInfoModal' -f components/factory/
echo "\nChecking for similar modal patterns:"
rg -l "getElementById.*showModal" --type tsx
Length of output: 508
Script:
#!/bin/bash
# Check DenomInfoModal implementation and usage with corrected commands
echo "Checking DenomInfoModal implementation:"
ast-grep --pattern 'export $_ function DenomInfoModal'
echo "\nChecking DenomInfoModal props usage:"
ast-grep --pattern 'interface DenomInfoModalProps'
echo "\nChecking for similar modal patterns:"
rg -l "getElementById.*showModal"
echo "\nChecking all DenomInfoModal usages:"
rg "DenomInfoModal"
Length of output: 1079
Script:
#!/bin/bash
# Let's check the DenomInfoModal implementation and its props interface
echo "Checking DenomInfoModal implementation in denomInfo.tsx:"
cat components/factory/modals/denomInfo.tsx
echo "\nChecking MyDenoms component implementation:"
cat components/factory/components/MyDenoms.tsx
Length of output: 22674
finalizes #13
Summary by CodeRabbit
New Features
DenomInfoModalis now always rendered in theTokenListcomponent, enhancing visibility and interaction regardless of the selected denomination.Bug Fixes