-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add the prompt "Are you satisfied with WooPayments?" for eligible merchants, behind feature flag #10370
base: develop
Are you sure you want to change the base?
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
Size Change: +1.61 kB (0%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
- Create merchant feedback prompt component - Show merchant feedback prompt on overview page with feature flag
…implify portal rendering
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.
Left some comments – mainly me exploring the different flags and getting a feel for this.
The key feedback (likely already on your radar, WIP!) is to clearly document the various flags (across plugin and service) and which are for production / launch vs to help us during development.
client/globals.d.ts
Outdated
@@ -17,6 +17,7 @@ declare global { | |||
paymentTimeline: boolean; | |||
isDisputeIssuerEvidenceEnabled: boolean; | |||
isPaymentOverviewWidgetEnabled?: boolean; | |||
isMerchantFeedbackPromptEnabled: boolean; |
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.
Is this the same as the server side eligibility flag (i.e. is this a placeholder)?
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.
This is a client-only dev flag, needed to allow us to push in-development code to trunk. I've renamed to clarify in in 6d0e870.
@@ -69,6 +70,9 @@ declare global { | |||
declineOnAVSFailure: boolean; | |||
declineOnCVCFailure: boolean; | |||
}; | |||
campaigns: { | |||
wporgReview2025: boolean; | |||
}; |
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.
Do we need to document campaigns
somewhere? Feels like a new concept. Also will all the children of it follow similar rules (e.g. inactive if MIA etc)?
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.
Should we use campaigns.wporgReview2025
to track if merchant has responded or dismissed?
If campaigns.wporgReview2025
stays boolean, how can we rename it so it's super obvious how it works / what it is?
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've added some comments in ed1e318 to clarify this.
@@ -393,4 +395,13 @@ private static function is_woopayments_gateway_enabled() { | |||
|
|||
return 'yes' === $woopayments_enabled_setting; | |||
} | |||
|
|||
/** | |||
* Checks if the merchant feedback prompt feature flag is enabled. |
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.
Might as well use this comment to clearly define/document this feature flag.
Is this a temporary feature flag, to allow us to ship the code and opt-in manually to test (e.g. on local or staging sites)?
When the dust settles on this PR will be useful to document the various flags at plugin and service level, and which are for development purposes vs. to manage eligibility and dismiss state (etc) after launch.
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've added a comment here and renamed this across the codebase to clarify that this is a temporary, development feature-flag in 6d0e870
* See https://github.com/WordPress/gutenberg/blob/c300edfebb48f79f6f0f6643ce04dd73303c5fcb/packages/components/src/snackbar/index.tsx#L119-L126 | ||
*/ | ||
content: ( | ||
<Flex |
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.
Using VStack
could be a simpler way to achieve the same layout
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.
Thanks @ciampo, TIL about VStack
!
Unfortunately, our outdated bundled version of @wordpress/components
(19.8.5) and the deprecated @types/wordpress__components
doesn't allow me to import VStack
or __experimentalVStack
, so I'd need to either override the type definitions manually or update the components or @types
library versions, both out of scope for this UI change.
I'd also be concerned about using an early (3+ years old) version of an experimental component, when Flex
seems to do the trick well enough.
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.
Got it. I strongly recommend updating @wordpress/*
dependencies, since, as you also mention, they are 3+ years old. The package has been shipping its own types for a while, on top of adding several improvements and features.
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.
Yes 100% agree @ciampo, these problems are only going to get worse as time goes on. This is a known issue that we're prioritising in these P2 posts:
Aligning WooPayments with the WordPress component package
and
Spike: How WooPayments imports, bundles, and overrides styles from WP components
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.
As already mentioned in the issue, snackbar is re-rendered on refresh of page even after dismissing, which will be fixed in #10329
PS @Automattic/helix if anyone approves this, please feel free to merge to help unblock subsequent project PRs 🚀 |
Resolves #10326
Partially resolves #10331 by adding tracks events for some interactions introduced in this PR
Changes proposed in this Pull Request
_wcpay_feature_prompt_merchant_for_review
is enabled – to be removed from client on launchwcpaySettings.accountStatus.campaigns.wporgReview2025
core/notices
snackbars are present.Pages where this is visible:
Temporary snackbar implementation / hack
@wordpress/components
version in WooPayments doesn't support explicit dismiss button (x
)@wordpress/components
Snackbar
/SnackbarList
doesn't allow two actions to be renderedcore/notices
createNotice()
only accepts a single action and no custom React markupcreateNotice()
snackbars are rendered.wp.components
versionImportant
This prompt is only visible if the development feature flag
_wcpay_feature_prompt_merchant_for_review
is enabledTesting instructions
Checkout the transact-platform-server PR https://github.com/Automattic/transact-platform-server/pull/7246this has been deployed_wcpay_feature_prompt_merchant_for_review
.update_option( '_wcpay_feature_prompt_merchant_for_review', '1' )
'wporgReview2025' => true
herenpm run changelog
to add a changelog file, choosepatch
to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge