-
Notifications
You must be signed in to change notification settings - Fork 197
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 Paid Course Upsell #5169
Add Paid Course Upsell #5169
Conversation
So we can have aligned texts for better demonstration
As suggested by Renatho.
So we can remove margins of the modal and remove the title from the header. Co-authored-by: Renatho De Carli Rosa <renatho@gmail.com>
So we can customize it easily. Co-authored-by: Renatho De Carli Rosa <renatho@gmail.com>
And fix pricing and pricing detail font sizes
So we can least look at how other editor wizard steps will behave. =)
So we can know that it applies only for the Course editor wizard easily.
So we can have these strings translated properly :D
So we don't need to go looking around for updating the price in the code..:)
So we can re-use the logic in the FeaturedProductSenseiPro component too! :D
Because I just observed that the image wasn't working in the final plugin build
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 looking great! I just added some small suggestions.
I accept suggestions of tests to do; =)
Just in order to merge it earlier, I'd do it in another PR. I'd test the following parts:
useSenseiProExtension
- We could mock theuseSelect
, and confirm if it's filtering the Sensei Pro properly.EditorWizardModal
- It'd be interesting to check if the upsell step is added when should and not when shouldn't. I think the easier way would be mocking theWizard
component, and checking if CourseUpgradeStep is in the array or not.CourseUpgradeStep
- We could also check if the price is rendered correctly and if the actions work properly.
Just one other thing while checking the design:
|
Instead of copying the array again without the step.
I had to add a resolver in the store to be able to load the extensions and so to identify the extension appropriately.
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 very good! Just one final thing.
After that, since it's pretty simple, I think you can merge directly.
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.
🎉🎉🎉
Great PR!
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 for the late review! It is looking nice but I found some "flashing" behaviour I reported in a comment.
Maybe we want to look into it.
if ( ! senseiProExtension || senseiProExtension.is_installed === true ) { | ||
stepsByPostType.course = stepsByPostType.course.filter( | ||
( step ) => step !== CourseUpgradeStep | ||
); | ||
} |
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 making the Upgrade step flash in/out if you go over the first step somewhat fast.
https://user-images.githubusercontent.com/799065/170244529-f19593a5-145b-42e3-baa5-b493d77157c7.mp4
Any idea on how to workaround that?
Fixes #5129
Changes proposed in this Pull Request
Testing instructions
Screenshot of the current version
To do
Questions