Fix: Restore claim disabled message modal overlay (#611)#626
Open
edehvictor wants to merge 2 commits intoGoodDollar:masterfrom
Open
Fix: Restore claim disabled message modal overlay (#611)#626edehvictor wants to merge 2 commits intoGoodDollar:masterfrom
edehvictor wants to merge 2 commits intoGoodDollar:masterfrom
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The temporary
useEffectthat unconditionally callsshowModal()on mount inOldClaimwill force the overlay to appear even whenclaimEnabledis true; consider gating it on!claimEnabledor removing it before merge so the modal only shows when the feature flag is actually disabled. - Now that the feature flag and modal wiring are restored, the inline comments like
// Uncommented per maintainer's instructionsand// Temporary useEffect...are no longer helpful for long-term maintenance and could be removed to keep the code clean.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The temporary `useEffect` that unconditionally calls `showModal()` on mount in `OldClaim` will force the overlay to appear even when `claimEnabled` is true; consider gating it on `!claimEnabled` or removing it before merge so the modal only shows when the feature flag is actually disabled.
- Now that the feature flag and modal wiring are restored, the inline comments like `// Uncommented per maintainer's instructions` and `// Temporary useEffect...` are no longer helpful for long-term maintenance and could be removed to keep the code clean.
## Individual Comments
### Comment 1
<location path="src/pages/gd/Claim/OldClaim.tsx" line_range="77-78" />
<code_context>
+ // Uncommented per maintainer's instructions
+ const { Dialog, showModal } = useDisabledClaimingModal(disabledMessage)
+
+ // Temporary useEffect per maintainer's instructions to trigger the modal
+ useEffect(() => {
+ showModal()
+ }, [showModal])
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider conditioning the initial modal display on `claimEnabled` to avoid showing it when claiming is allowed.
Right now this effect triggers `showModal()` on every mount, so users will see the "Claiming Unavailable" modal even when claiming is enabled. To align behavior with the flag, gate the effect body and track the flag in the deps:
```ts
useEffect(() => {
if (!claimEnabled) {
showModal();
}
}, [claimEnabled, showModal]);
```
Suggested implementation:
```typescript
// Temporary useEffect per maintainer's instructions to trigger the modal
useEffect(() => {
if (!claimEnabled) {
showModal()
}
}, [claimEnabled, showModal])
```
These changes assume that `claimEnabled` is already defined in the component scope (e.g. from props, context, or derived from `supportedChains`). If it is not yet defined, you'll need to:
1. Introduce a `const claimEnabled = ...` expression (or prop) that reflects whether claiming is currently allowed.
2. Ensure `claimEnabled` is correctly updated when the underlying feature flag or conditions change so the effect re-runs appropriately.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
Hey @L03TJ3 and @sirpy , I've just submitted the PR for the claim disabled modal fix (Issue #611)! 🚀 To fix the layout squeezing, I completely removed the problematic useModal hook and built a clean, absolute-positioned overlay. I also wrapped the claim container in position="relative" and overflow="hidden" so the dark grey backdrop covers the area perfectly on both mobile and desktop without being dismissable. |
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.
Description
The "Claiming Unavailable" modal was previously broken because the
useModalhook implementation caused the dialog to participate in the flex layout, which squeezed it and pushed it behind other elements.This PR completely removes the problematic
useModalhook inuseDisabledClaimModal.tsxand replaces it with a dedicated, absolutely-positioned<Box>overlay (zIndex: 50) with a semi-transparent dark backdrop. To ensure the overlay perfectly covers only the claim interaction area without breaking the rest of the page,position="relative"andoverflow="hidden"were added to the parentbalanceContainerinOldClaim.tsx. The modal is un-dismissable, meeting all acceptance criteria.About #611
How Has This Been Tested?
claim-featureflag tofalseto trigger the disabled state.Checklist:
Screenshots
Summary by Sourcery
Restore and harden the disabled-claim modal so it properly overlays the claim area and blocks claiming when the feature flag is off.
Bug Fixes:
Enhancements: