Repository navigation
Revert "fix: transaction list UI polish" - #584
Closed
gringokiwi wants to merge 1 commit into
Closed
gringokiwi wants to merge 1 commit into
gringokiwi wants to merge 1 commit into
Conversation
Contributor
WalkthroughUpdates Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Deploying wallet-bitcoin with
|
| Latest commit: |
63285ef
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a10bf33e.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://revert-555-wt-tx-list-fixes.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
63285ef
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://558eb03d.arkade-wallet.pages.dev |
| Branch Preview URL: | https://revert-555-wt-tx-list-fixes.arkade-wallet.pages.dev |
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
63285ef
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a7c49dcb.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://revert-555-wt-tx-list-fixes.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/TransactionsList.tsx`:
- Around line 227-239: The list container in TransactionsList.tsx uses a fixed
height calc(100dvh - 380px) and doesn't account for the newly added heading
(TextLabel/Focusable block), which can cause overflow; update the container (the
div with ref parentRef and handlers handleListKeyDown/focusOnFirstRow) to either
subtract the heading's height from the calc (e.g., reduce the constant by the
heading's pixel height) or refactor the parent to a flex column so the header
takes its natural height and the list uses flex: 1 to consume remaining
space—choose one approach and apply it consistently so the list never exceeds
viewport height on short screens.
In `@src/lib/types.ts`:
- Line 71: Enum member Haptics currently uses the label 'haptics feedback' which
is a typo; update the enum value for Haptics in src/lib/types.ts to a standard
label such as 'haptic feedback' (or 'haptics') by replacing the string literal
for the Haptics member, and if any consumers rely on the previous exact string,
update those usages to the new label.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7fb29ef-9156-4a40-96dd-7a9b0ade8259
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
src/components/TransactionsList.tsxsrc/lib/types.ts
Comment on lines
+227
to
+239
| <> | ||
| <TextLabel>Transaction history</TextLabel> | ||
| <Focusable id='outer' onEnter={focusOnFirstRow} ariaLabel={ariaLabel()}> | ||
| <div | ||
| ref={parentRef} | ||
| onKeyDown={handleListKeyDown} | ||
| className='hide-scrollbar scroll-fade' | ||
| style={{ | ||
| borderBottom: border, | ||
| height: 'calc(100dvh - 380px)', | ||
| minHeight: '200px', | ||
| overflowY: 'auto', | ||
| }} |
Contributor
There was a problem hiding this comment.
Adjust the list height for the new heading.
This adds a new block above a container that still uses a fixed calc(100dvh - 380px) height. On shorter viewports, the extra heading height will push the list past the available space and can reintroduce page-level overflow/nested scrolling. Please subtract the heading height here or switch this section to a flex column so the list consumes the remaining space.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/TransactionsList.tsx` around lines 227 - 239, The list
container in TransactionsList.tsx uses a fixed height calc(100dvh - 380px) and
doesn't account for the newly added heading (TextLabel/Focusable block), which
can cause overflow; update the container (the div with ref parentRef and
handlers handleListKeyDown/focusOnFirstRow) to either subtract the heading's
height from the calc (e.g., reduce the constant by the heading's pixel height)
or refactor the parent to a flex column so the header takes its natural height
and the list uses flex: 1 to consume remaining space—choose one approach and
apply it consistently so the list never exceeds viewport height on short
screens.
| Backup = 'backup', | ||
| General = 'general', | ||
| Haptics = 'haptic feedback', | ||
| Haptics = 'haptics feedback', |
Contributor
There was a problem hiding this comment.
Fix the settings label copy.
"haptics feedback" reads like a typo and will show up directly in the Settings UI. Please keep the standard wording here ("haptic feedback" or just "haptics").
Suggested fix
- Haptics = 'haptics feedback',
+ Haptics = 'haptic feedback',📝 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.
Suggested change
| Haptics = 'haptics feedback', | |
| Haptics = 'haptic feedback', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/types.ts` at line 71, Enum member Haptics currently uses the label
'haptics feedback' which is a typo; update the enum value for Haptics in
src/lib/types.ts to a standard label such as 'haptic feedback' (or 'haptics') by
replacing the string literal for the Haptics member, and if any consumers rely
on the previous exact string, update those usages to the new label.
Collaborator
|
Closing, no need to revert. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reverts #555
Summary by CodeRabbit
Bug Fixes
UI/Style Improvements