Replace AppButtonModalTrigger with useToggle hook.#3514
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3514 +/- ##
============================================
- Coverage 67.6% 64.2% -3.5%
============================================
Files 518 395 -123
Lines 21461 6793 -14668
Branches 0 1731 +1731
============================================
- Hits 14515 4358 -10157
+ Misses 6946 2174 -4772
- Partials 0 261 +261
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR removes the AppButtonModalTrigger component (which relied on cloneElement to inject onRequestClose) and replaces that pattern with an explicit useToggle hook plus local open-state wiring at each modal call site, making modal open/close behavior visible where it’s used.
Changes:
- Introduces
useToggle(with unit tests) to manage boolean open/close state. - Refactors multiple modal trigger call sites to render buttons + conditionally render modals with explicit
onRequestClose. - Extracts small per-row components in table
.map()contexts to keep hook usage compliant with React’s rules of hooks.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| js/src/pages/product-feed/issues-table-card/issues-table-data.js | Extracts IssueReadMoreButton to manage modal open/close explicitly via useToggle. |
| js/src/pages/dashboard/all-programs-table-card/edit-program-button/index.js | Replaces trigger component with explicit state and conditional EditProgramPromptModal rendering. |
| js/src/pages/attribute-mapping/attribute-mapping-table.js | Replaces trigger component in table actions/footer; extracts per-row buttons using useToggle; preserves modal open/close tracking events. |
| js/src/hooks/useToggle.js | Adds a reusable boolean toggle hook for modal open state. |
| js/src/hooks/useToggle.test.js | Adds unit tests covering default state, custom initial value, toggling behavior, and stable function reference. |
| js/src/components/shipping-rate-section/estimated-shipping-rates-card/shipping-rate-input-control.js | Replaces trigger component with explicit modal open state and passes onRequestClose. |
| js/src/components/shipping-rate-section/estimated-shipping-rates-card/estimated-shipping-rates-card.js | Replaces trigger component for “Add another rate” with explicit useToggle modal state. |
| js/src/components/order-value-condition-section/minimum-order-card/minimum-order-input-control.js | Replaces trigger component for edit modal with explicit useToggle state and onRequestClose. |
| js/src/components/order-value-condition-section/minimum-order-card/minimum-order-card.js | Replaces trigger component for “Add another condition” modal with explicit useToggle state and onRequestClose. |
| js/src/components/app-button-modal-trigger.js | Removes the cloneElement-based trigger component entirely. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <AppButton | ||
| { ...buttonProps } | ||
| isLink | ||
| className={ classnames( className ) } | ||
| onClick={ toggleModal } |
There was a problem hiding this comment.
EditProgramButton is purpose-specific, not generic. It has one job: open an edit modal. The onClick override is intentional: the component owns its click behavior, not the caller.
| const useToggle = ( initialValue = false ) => { | ||
| const [ value, setValue ] = useState( initialValue ); | ||
| const toggle = useCallback( () => setValue( ( prev ) => ! prev ), [] ); | ||
| return [ value, toggle ]; | ||
| }; |
There was a problem hiding this comment.
The name toggle explicitly implicates that the function should be to toggle on/off. I don't think there's a strong case for accepting a boolean as a parameter here.
Changes proposed in this Pull Request:
Closes GOOWOO-56
This PR replaces
AppButtonModalTriggerwith a newuseTogglehook, addressing the hidden coupling issue described in the ticket.Previously,
AppButtonModalTriggerusedReact.cloneElementto silently injectonRequestCloseinto whatever modal was passed to it. This made it impossible to understand how a modal's close behaviour worked without tracing all the way back toAppButtonModalTrigger— the dependency was invisible from the modal component's props.The new
useTogglehook returns a[ value, toggle ]tuple (with an optional initial value) and lets each consumer wire uponClickandonRequestCloseexplicitly. The open state is visible at the call site, removing the hidden coupling.Note: For the two cases where the modal trigger lives inside a
.map()loop (issues-table-data.js,attribute-mapping-table.js), small private components (IssueReadMoreButton,EditRuleButton,DeleteRuleButton) were extracted to keep hooks at the component level, as required by React's rules of hooks.Screenshots:
N/A — no visual changes. All existing behaviour is preserved.
Detailed test instructions:
gla_modal_open/gla_modal_closed) still fire correctly.Additional details:
Changelog entry