New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Show feedback footer on product editor page #38599
Conversation
Test Results SummaryCommit SHA: 4d218c6
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. |
Hi , @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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really like the use of the new hook!
Left a few comments, mostly opinionated nitpicks around naming.
> | ||
<WooFooterItem.Slot name="product" /> | ||
|
||
<FeedbackBar product={ product } /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional, but it might be nice to move the FeedbackBar
and ProductMVPFeedbackModalContainer
to a fill for WooFooterItem
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FeedbackBar actually already is a fill. It feels odd to include it here, but seemed like the most natural place to do so. Any suggestions?
packages/js/product-editor/src/hooks/use-feedback-bar/use-feedback-bar.ts
Outdated
Show resolved
Hide resolved
packages/js/product-editor/src/components/feedback-bar/feedback-bar.tsx
Outdated
Show resolved
Hide resolved
packages/js/product-editor/src/components/feedback-bar/feedback-bar.tsx
Outdated
Show resolved
Hide resolved
packages/js/product-editor/src/hooks/use-feedback-bar/use-feedback-bar.ts
Show resolved
Hide resolved
Ready for another round of review, @joshuatf, assuming the checks pass. 🤞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the refactor! Really nice work, @mattsherman.
I left a couple thoughts, but those don't need to be addressed in this PR. Just one comment about using the window tracking flag to avoid fetching.
Otherwise this PR is testing perfectly and ready to go after rebase.
}; | ||
|
||
const onShareFeedbackClick = () => { | ||
recordEvent( 'product_editor_feedback_bar_share_feedback_click', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not something we need to do in this PR, but just thinking out loud about a helper util in the future: something like recordProductEditorEvent( eventName )
might be nice that handles prefixing with product_editor_
and adding in entity props such as the product_type
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of auto-adding entity props.
I'm not sure about prefixing the Tracks event name... doing dynamic generation of event page makes it harder to find events in code when searching. But, I'm not totally opposed to it.
PRODUCT_EDITOR_SHOW_FEEDBACK_BAR_OPTION_NAME | ||
) as string; | ||
|
||
const allowTrackingOption = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this on the first pass, but we have window.wcTracks.isEnabled
on the window that we can grab instead of fetching the option.
Same comment for areas getting the tracking option below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, cool.... I'll make that change. So, in general, should we never have to get the tracking option client-side by fetching it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. The only case where it could become out of sync is when it gets enabled client-side without a refresh, but that won't happen if anyone doing this is hitting the correct tracks API (window.wcTracks.enable
) to enable this:
window.wcTracks.isEnabled = true; |
a7c6633
to
4d218c6
Compare
@joshuatf Rebased and updated to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with and without tracks enabled and still working as expected! Thanks for all the effor on this feature, Matt!
LGTM 🚢
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes #38519.
No footer (ignore section backgrounds; see #38607)
Footer
Footer with overlapping transient notice (to be addressed outside of this PR)
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_bar
woocommerce_ces_shown_for_actions
Note: For the following, you will want to reset the state of the system between verifications to get the feedback footer to show (see above).
wcadmin_product_editor_feedback_bar_dismiss_click
Tracks event is loggedwcadmin_product_editor_feedback_bar_share_feedback_click
Tracks event is loggedwcadmin_product_editor_feedback_bar_turnoff_editor_click
Tracks event is logged