fix: ibc chain dropdown closes on selection #75
Conversation
WalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/bank/components/sendBox.tsx(0 hunks)components/bank/forms/ibcSendForm.tsx(4 hunks)
💤 Files with no reviewable changes (1)
- components/bank/components/sendBox.tsx
🧰 Additional context used
📓 Learnings (1)
components/bank/forms/ibcSendForm.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/bank/forms/ibcSendForm.tsx:149-152
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In `components/bank/forms/ibcSendForm.tsx`, the `handleSend` function relies on the `useTx` hook for error handling, so additional error handling or user notifications within `handleSend` are unnecessary.
🔇 Additional comments (2)
components/bank/forms/ibcSendForm.tsx (2)
Line range hint 1-17: LGTM! Clean up of unused imports and props
The removal of the useEffect import and setIsIbcTransfer prop helps streamline the component by removing unused code.
171-171: LGTM! Form validation and submission logic is solid
The form validation and submission logic is well-implemented with proper error handling through the useTx hook, as confirmed by previous learnings.
| onClick={e => { | ||
| setSelectedChain(chain.id); | ||
| // Get the dropdown element and remove focus | ||
| const dropdown = (e.target as HTMLElement).closest('.dropdown'); | ||
| if (dropdown) { | ||
| (dropdown as HTMLElement).removeAttribute('open'); | ||
| (dropdown.querySelector('label') as HTMLElement)?.focus(); | ||
| (dropdown.querySelector('label') as HTMLElement)?.blur(); | ||
| } | ||
| }} | ||
| onKeyDown={e => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault(); | ||
| setSelectedChain(chain.id); | ||
| // Get the dropdown element and remove focus | ||
| const dropdown = (e.target as HTMLElement).closest('.dropdown'); | ||
| if (dropdown) { | ||
| (dropdown as HTMLElement).removeAttribute('open'); | ||
| (dropdown.querySelector('label') as HTMLElement)?.focus(); | ||
| (dropdown.querySelector('label') as HTMLElement)?.blur(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve dropdown accessibility and code maintainability
While the changes fix the dropdown closure issue, there are some improvements we can make:
- Replace direct DOM manipulation with React refs
- Update ARIA attributes for better accessibility
- Extract duplicate logic to a shared function
Here's the suggested implementation:
+ const dropdownRef = useRef<HTMLDivElement>(null);
+ const labelRef = useRef<HTMLLabelElement>(null);
+
+ const handleChainSelection = (chain: IbcChain) => {
+ setSelectedChain(chain.id);
+ if (dropdownRef.current) {
+ dropdownRef.current.removeAttribute('open');
+ labelRef.current?.focus();
+ labelRef.current?.blur();
+ }
+ };
<div
+ ref={dropdownRef}
className="dropdown dropdown-end w-full">
<label
+ ref={labelRef}
tabIndex={0}
aria-label="chain-selector"
role="combobox"
- aria-expanded="false"
+ aria-expanded={dropdownRef.current?.hasAttribute('open') || false}
aria-controls="chain-dropdown"
aria-haspopup="listbox"
className="btn btn-md btn-dropdown w-full">
<a
- onClick={e => {
- setSelectedChain(chain.id);
- const dropdown = (e.target as HTMLElement).closest('.dropdown');
- if (dropdown) {
- (dropdown as HTMLElement).removeAttribute('open');
- (dropdown.querySelector('label') as HTMLElement)?.focus();
- (dropdown.querySelector('label') as HTMLElement)?.blur();
- }
- }}
+ onClick={() => handleChainSelection(chain)}
onKeyDown={e => {
if (e.key === 'Enter' || e.key === ' ') {
e.preventDefault();
- setSelectedChain(chain.id);
- const dropdown = (e.target as HTMLElement).closest('.dropdown');
- if (dropdown) {
- (dropdown as HTMLElement).removeAttribute('open');
- (dropdown.querySelector('label') as HTMLElement)?.focus();
- (dropdown.querySelector('label') as HTMLElement)?.blur();
- }
+ handleChainSelection(chain);
}
}}Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
==========================================
- Coverage 59.00% 58.98% -0.02%
==========================================
Files 145 145
Lines 14060 14072 +12
==========================================
+ Hits 8296 8301 +5
- Misses 5764 5771 +7 ☔ View full report in Codecov by Sentry. |
fixes #14
Summary by CodeRabbit
New Features
IbcSendFormcomponent.Bug Fixes
setIsIbcTransferfrom theIbcSendForm, simplifying the component's interface.Refactor
useEffectimport inIbcSendForm.