Fix: Restore claim disabled message modal overlay (#611)#626
Fix: Restore claim disabled message modal overlay (#611)#626L03TJ3 merged 4 commits intoGoodDollar:masterfrom
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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.
|
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. |
|
@edehvictor |
|
Hey @L03TJ3 , thanks for the review! I've pushed the updates: Grey Overlay Removed: Dropped the background color. The wrapper remains as an absolute, invisible shield (zIndex: 50), successfully blocking all clicks to the Claim button. Image Fixed: Updated the See the updated screenshot below. Let me know if we're good to go! 
|
|
@L03TJ3, please review the changes I made |
src/pages/gd/Claim/OldClaim.tsx
Outdated
| // const [, payload] = useFeatureFlagWithPayload('claim-feature') | ||
| // const { enabled: claimEnabled = true, disabledMessage = '' } = (payload as any) || {} | ||
|
|
||
| // Uncommented per maintainer's instructions |
There was a problem hiding this comment.
| // Uncommented per maintainer's instructions |
These type of comments don't have any value. if you think its valuable to let us know it can be as comment in the github PR.
Comments are to explain a certain flow, explain edge cases or just in general developer guidance which might not be 100% clear by just reading the code
|
|
||
| return supportedChains.map((chainId) => chainNames[chainId] || `Chain ID: ${chainId}`).join(', ') | ||
| }, [supportedChains]) | ||
|
|
There was a problem hiding this comment.
Why have you removed valid comments?
with what goal?
|
@L03TJ3 sorry about that, I'll revert the valid comments and delete the invalid once |
|
@L03TJ3, I've just pushed a commit that restores all the original, valid comments and removes the unhelpful ones I had added. Let me know if everything looks good to go now |
1 similar comment
|
@L03TJ3, I've just pushed a commit that restores all the original, valid comments and removes the unhelpful ones I had added. Let me know if everything looks good to go now |
|
Hey @edehvictor thanks for the contribution. Make sure to do your request payment in gardens |
2 participants
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: