Add/segmentation to jetpack products my jetpack #38283
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Backup plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Starter Plugin plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Migration plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
github-actions
bot
added
the
[Status] Needs Author Reply
projects/packages/my-jetpack/_inc/components/product-cards-section/index.tsx
Outdated
Show resolved
Hide resolved
projects/packages/my-jetpack/_inc/components/product-cards-section/style.module.scss
Show resolved
Hide resolved
projects/packages/my-jetpack/_inc/components/product-cards-section/style.module.scss
Outdated
Show resolved
Hide resolved
github-actions
bot
added
[Plugin] Backup
There was a problem hiding this comment.
The code looks good! Very nice implementation. I just left two remarks below.
The design could use a bit tinkering, at least on my screen:
The spacing is off, and the color of "Discover more" background should be white (not beige as currently). Here is the design for the reference:
projects/packages/my-jetpack/_inc/components/product-cards-section/index.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| return ( | ||
| <Col tagName="li" sm={ 4 } md={ 4 } lg={ 4 } key={ product }> | ||
| <Item admin={ !! userIsAdmin } /> |
There was a problem hiding this comment.
Is double-negation necessary here?
There was a problem hiding this comment.
userIsAdmin is a string that is either "1" or "0" so I think we should keep it to be safe
There was a problem hiding this comment.
How about not doing implicit casting here and just check userIsAdmin === '1'? I think typescript would complain about that one if it knew about userIsAdmin type 😅
There was a problem hiding this comment.
userIsAdmin is a string that is either "1" or "0" so I think we should keep it to be safe
Quick heads up that in JavaScript, only empty strings are falsy, so both strings "0" and "1" are considered valid truthy values.
CodeyGuyDylan
added
the
[Status] Needs Design Review
I went ahead and made it so the background was white everywhere except the My Products section and that fixed the spacing issues in turn. Just took some markup rework 😄 |
There was a problem hiding this comment.
Thanks for working on the changes @CodeyGuyDylan ! It looks great. I'm happy you managed to work around the change of background.
One last thing, it seems like some
is added here, and it impacts the spacing of the sections:
Besides that, it's a 🟢
|
Thank you for this amazing work and the ping, @CodeyGuyDylan! It looks great. Everything works as expected. Here are my thoughts:
|
|
As for the spacing/blending between the header and the content when there are any notifications on top of the active products, can we add a light border at the bottom of the header (Gray 5)? This would create more structure to the page. E.g.: More design examples are here: https://www.figma.com/design/g1ZLnS65PzfOL9cmByykvf/My-Jetpack%3A-Separating-active-and-inactive-products?node-id=4444-302&t=jgzGabaV3KKEDOeH-4 |
Will get that line added!
Creator gets activated as soon as you user-connect your account. I am actually unsure how to disable it from settings 🤔 Do you have any idea @robertsreberski?
Yes that is correct, Stats only needs a site connection!
We have a status where if a plugin is installed but not active, you can activate it from My Jetpack. It must not be working for VideoPress and Social 🤔 I will try and debug why that is happening |
@ilonagl Seems like we show this when the product has a free or paid plan and the plugin is installed but not active. I think the intention here is for the interstitial handle giving the free plan or offering the paid one, so I think all we have to do is get that added for VideoPress and Social 🤔 I'll add a maintenance item for this 😄 |
CodeyGuyDylan
force-pushed
the
add/segmentation-to-jetpack-products-my-jetpack
branch
from
17ae5ab to
1159488
Compare
|
A little bit of extra information on these interstitials, the reason there is no free offering is this: In the code, only the product interstitial tables support free offerings without a plan. Products like Search, which shows a "Start for free" CTA on the card, go to checkout to "purchase" the free plan. So we'll have to rework that a bit to support the free offering without a plan on product cards as well 😄 |
| { slugs.map( product => { | ||
| if ( product === 'stats' && showFullJetpackStatsCard ) { | ||
| return null; | ||
| } | ||
|
|
||
| const Item = items[ product ]; | ||
| if ( ! Item ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Nitpick: to simplify the render of the cards, we can create a new array filtering out when the product is 'stats' or when there's no items[ product ]. Then, the .map function would be only responsible for returning the array of <Col> with each Item
| 'ownedProducts' => Products::get_products_by_ownership()['owned'], | ||
| 'unownedProducts' => Products::get_products_by_ownership()['unowned'], |
There was a problem hiding this comment.
What do you think if pass the type of ownership as an argument so we can retrieve the specific values that we want at any time? Alternatively, if we are only going to call this method here, we can store the return value in a variable and use it afterwards:
$products_by_ownership = Products::get_products_by_ownership();
//...
'ownedProducts' => $products_by_ownership['owned'],
'unownedProducts' => $products_by_ownership['unowned'],There was a problem hiding this comment.
Good idea, I'll get that changed, thank you!
@CodeyGuyDylan what do you mean by disabling it from settings? I think we could improve the code by changing the value returned from |
|
Thank you for looking into my comments and the follow-ups, @CodeyGuyDylan! I agree with @robertsreberski that Creator shouldn't be active by default; I left a related comment, but it's out of the scope of your specific project. If there is anything else I can help with this work, or if you want me to take a second look once we have the header divider, just let me know! |
|
Thank you for your input @robertsreberski and @ilonagl, we decided today in our meeting not to do anything about the Creator card for now since it will soon be replaced with Newsletter As far as the interstitials not offering a free version for VideoPress and Social, we have a project on the roadmap to make interstitials more consistent, I think we will probably go ahead and handle this issue during that project 😄 |
|
Sounds great! Thank you for covering all of this, even though it's outside the project you're working on. 🙌 |
|
It's interesting that if I run this PR in a local dev environment (via Docker), Boost and CRM are showing in the Discover More section... 🤔?. In my local environment, ALL the plugins are installed; Akismet and Jetpack are activated. But since ALL the plugins are installed (including Boost and CRM), shouldn't ALL the products show under the My Products section? See Screenshot: |
Ah nice catch, that would probably be something that @elliottprogrammer can look at in one of his Protect PRs 😄 |
CodeyGuyDylan
deleted the
add/segmentation-to-jetpack-products-my-jetpack
branch
github-actions
bot
removed
[Status] Design Input Requested
[Status] Needs Design Review
Yes, @IanRamosC and @CodeyGuyDylan, I was aware of this and It has been fixed in #38332 but is still in the review process. 👍 |
Proposed changes:
Note that this will be different from products that have an "active" versus "inactive" status. A user may be considered as "having" or "owning" a product if any of the following are true:
Other information:
Jetpack product discussion
P2: p1HpG7-sMW-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions: