Fix transient notices overlapping product editor footer (take 2) #38698
Conversation
github-actions
bot
added
focus: react admin [team:Ghidorah]
plugin: woocommerce
mattsherman
added
focus: product
mattsherman
changed the title
mattsherman
changed the title
|
Hi @joshuatf, @woocommerce/mothra Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
| { /* We put Footer here instead of in InterfaceSkeleton because Footer uses | ||
| WooFooterItem to actually render in the WooFooterItem.Slot defined by | ||
| WooCommerce Admin. And, we need to put it outside of the SlotFillProvider | ||
| we create in this component. */ } | ||
| <Footer product={ product } /> |
There was a problem hiding this comment.
@joshuatf Do you think we should remove the SlotFillProvider from this component and require the consumer of it to make sure it is in a SlotFillProvider?
Otherwise, any blocks/components in in the product editor won't be able to use fill slots in WooCommerce Admin (since the slot don't be in the context of this SlotFillProvider). Or, is that actually a good thing?
There was a problem hiding this comment.
Definitely! Good call. Do you want to tackle here or in a follow up?
There was a problem hiding this comment.
I'll handle this in a follow-up, in case there are any details that cause that to blow things up a bit.
Test Results SummaryCommit SHA: 4a45848
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
|
Oops, failing test -- probably due to lack of SlotFillProvider (so notices are not rendered). I'll update the tests later and put it back up for review! |
|
Fixed failing unit tests... mocking was a little tricky to figure out, due to slot-fill. In the end, mocking |
There was a problem hiding this comment.
Nicely done, Matt! Love how clean this implementation is now.
Left one optional comment and agree with your suggestion on the SlotFillProvider. Pre-approving pending whether or not you want to add those changes here. If so, feel free to re-ping me and I can re-approve.
| @@ -93,42 +92,40 @@ export function FeedbackBar( { product }: FeedbackBarProps ) { | |||
| return ( | |||
| <> | |||
| { shouldShowFeedbackBar && ( | |||
There was a problem hiding this comment.
Optional and obviously not a part of this PR, but since this code is being touched: I really like the following pattern for readability and reducing indentation.
if ( ! shouldShowFeedbackBar ) {
return null;
}
return <>{/* Feedbackbar render */}</>;
There was a problem hiding this comment.
Agreed. Will try to do this on future code.
| return { | ||
| __esModule: true, // Use it when dealing with esModules | ||
| ...originalModule, | ||
| WooFooterItem: jest.fn( ( { children } ) => { |
There was a problem hiding this comment.
Nice! This is always a little of a pain to mock. Would be nice to get some utils around slot fills to make this easier on us in the future.
| { /* We put Footer here instead of in InterfaceSkeleton because Footer uses | ||
| WooFooterItem to actually render in the WooFooterItem.Slot defined by | ||
| WooCommerce Admin. And, we need to put it outside of the SlotFillProvider | ||
| we create in this component. */ } | ||
| <Footer product={ product } /> |
There was a problem hiding this comment.
Definitely! Good call. Do you want to tackle here or in a follow up?
Submission Review Guidelines:
Changes proposed in this Pull Request:
In #38599, the product editor footer was introduced.
Transient notices overlapped this new footer.
This PR updates the the transient notices to not overlap the WooCommerce Admin footer.
To support this change, the
TransientNoticescomponent was changed to use theWooFooterItemfill. The CSS was updated to account for this change. No visible changes should be introduced to the use ofTransientNoticesanywhere in WooCommerce except for the case this PR is addressing (not overlapping the footer).Closes #38651.
Before (overlapping transient notices)
After (transient notices above footer)
How to test the changes in this Pull Request:
Note: To reset the state of the system so that the feedback footer will be shown, delete the following options:
woocommerce_product_editor_show_feedback_barwoocommerce_ces_shown_for_actions