Add logic of Upgrade Notice after upgrading Products to Product Collection #10267
Conversation
Implement the logic to display option for manual upgrade.Implement the logic to display option for manual upgrade. before enabling automatic upgrade and in case of reverting manual upgrade. 🚀 This comment was generated by the automations bot based on a
|
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
assets/js/blocks/migration-products-to-product-collection/migration-from-product-collection-to-products.ts
assets/js/blocks/migration-products-to-product-collection/migration-from-products-to-product-collection.ts assets/js/blocks/product-collection/inspector-controls/index.tsx assets/js/blocks/product-query/inspector-controls.tsx |
|
Size Change: +12.9 kB (+1%) Total Size: 1.35 MB
ℹ️ View Unchanged
|
Read from local storage.Read from local storage. If reverted - don't subscribe 🚀 This comment was generated by the automations bot based on a
|
…ntrols of Product Colelction
kmanijak
changed the title
kmanijak
added
block-type: product-query
kmanijak
added
the
skip-changelog
woocommercebot
requested review from
a team and
gigitux
and removed request for
a team
I was testing this, and I have one question. For example:
CC: @jarekmorawski |
There was a problem hiding this comment.
Hey @kmanijak, great job, and thank you so much for the detailed testing steps! 🙌🏻
Almost everything is working as expected. I left a few minor comments 🙂
There was a problem hiding this comment.
The UI for the Notice isn't looking great because there is a significant amount of white space that we are not utilizing properly. However, I don't believe we can improve it unless we contribute upstream. Here is how the UI is appearing on my machine:
I suggest adding some margin at the bottom of the notice. What do you think?
There was a problem hiding this comment.
Good feedback! I agree that the notice could use some breathing room. Also, can we use a different background color? Maybe Gutenberg-100? We should definitely contribute to removing the white space on the right if we can.
There was a problem hiding this comment.
Here is how the UI is appearing on my machine:
Thanks for raising this. I actually provided the fix upstream in Gutenberg, but it's still in review: WordPress/gutenberg#52240. It was also raised in previous PRs, where the notice was introduced and I consulted with @danieldudzic as well as @gigitux (e.g. here) if should introduce the temporary fix in the WC blocks and we agreed it's better to fix it upstream. But now it's been couple of weeks since the fix was submitted and no movement there. Under such circumstances do you think I should add a temporary fix in this PR?
Also, can we use a different background color? Maybe Gutenberg-100?
I remember that was the design you provided, but Gutenberg's Notice component doesn't support such a color. Is it ok to customize it on a WC Blocks level or is it preferable to keep using whatever Gutenberg provides to keep the UI unified?
There was a problem hiding this comment.
@jarekmorawski, I'll be merging this PR, but there's still some time to apply some changes before it's released. Please let me know your thoughts on the above comment 🙏
Good question. This notice should appear only for existing Products (Beta) blocks, and the user should not be able to add any new ones. We'd only show it for the first 3-5 visits to the editor (or until dismissed manually), so when the user checks back in a month, they wouldn't be able to revert to Products (Beta). |
Thanks for your input @jarekmorawski. Yes, the idea is that when we enable automatic migration, then Products (Beta) block won't be available in the inserter, unless merchant reverted the migration.
My suggestion is to add two conditions:
Whichever happens first, we don't show the notice anymore. What do you think about that @imanish003, @jarekmorawski? Anyway, my idea was to implement this last bit in a separate PR, but great we have that discussion. |
|
Hey Karol!
Sounds good to me, except IMO, visits should be calculated only when:
This way, we show the notice at least 3-5 times to the user unless the user dismisses it. But this is just my opinion. I think we can go with whatever @jarekmorawski approves 🙂 |
|
@jarekmorawski , @shaunandrews , could we get from either of you feedback on this thread. Let me summarise it: When users migrated the Products (Beta) block to Product Collection (for now manually), we display the Notice: We need to agree on conditions where to hide it. Currently implemented are:
But we cannot display the Notice forever. As @jarekmorawski wrote:
I suggested we add two conditions:
Does it make sense? Do you agree with these values: 24h, 4 visits? |
|
Update: the above solution is mostly covered in #10494, but awaits the sign off. |
|
Hi @kmanijak 👋 The first condition looks good to me. I'd tweak the second one to 72 hrs because merchants may not visit the editor very often when they're busy running a business. I'd also follow @imanish003's advice to only count a visit when the block is in focus and the sidebar is open. Thanks for handling this! |
* Add timestamp to each upgrade notice status change * Revert back only Product Collections converted from Products block * Make the time threshold configurable * Add logic that hides the Upgrade Notice after some amount of displays * Fix the taxQuery migration from Product Collection to Products * Change the way to count Product Collection entries * Fix the problem of multiple display counter increments with Product Collection * Update Upgrade Notice visibility conditions * Add contiions to unmark Product Collection as converted from Products * Change variable name * Change variable t to time name for better readibility. Improve types * Replace useState with useRef * Remove unecessary props passed to UpgradeNotice
|
@gigitux , @imanish003 can I ask either of you to review this PR? This PR contains changes from #10494 that were tested separately and now it provides the full-fledged migration from Products (Beta) to Product Collection. I tried to request a review from you, but this PR is already awaiting review from you hence the comment. Thank you! 🙏 |
|
Hey @kmanijak 👋 It appears that the testing steps might be out of date. Considering that PR #10494 has been merged into this one, it seems necessary to include information about when the notice will disappear i.e.
I've tested this PR and haven't come across any issues. Once the testing steps are updated, I can review it again. Thanks 🙌🏻 |
When a user dismisses the notice. Thanks @imanish003! Good catch! I updated the testing steps by adding more information to SCENARIO 1 🙌 Could you check it again, please? |
There was a problem hiding this comment.
Hey Karol! I appreciate your efforts in updating the testing steps. I've observed that in the section labeled SCENARIO 2. Dismiss the notice, the following step might be inaccurate:
Continue to follow steps 7 - 12.
Could you please review this step once more?
Aside from this, I find everything else to be in good order. You've done an excellent job. Thank you for incorporating the automatic migration. 🙌🏻 🚀
This PR adds a logic of displaying an Upgrade Notice after the automatic update of Products block to Product Collection with the following rules:
This PR contains changes from #10494 that were tested separately.
Fixes #9703
Testing
Automated Tests
User Facing Testing
Prerequisites:
trueSCENARIO 1. Acknowledge and ignore the Upgrade Notice
EXTRA STEPS (covered and tested in scope of Product Collection - logic to hide upgrade notice #10494):
SCENARIO 2. Dismiss the notice
The information about the notice state is kept in Local Storage. Before attempting another scenario, we have to clear that. In order to do that go to Dev Tools > Application > Local Storage and delete
wc-blocks_upgraded-products-to-product-collection(Chrome).The journey may differ between browsers, so please check the way to delete Local Storage entries in your browser.
SCENARIO 3. Revert the migration
The information about the notice state is kept in Local Storage. Before attempting another scenario, we have to clear that. In order to do that go to Dev Tools > Application > Local Storage and delete
wc-blocks_upgraded-products-to-product-collection(Chrome).The journey may differ between browsers, so please check the way to delete Local Storage entries in your browser.
WooCommerce Visibility