-
Notifications
You must be signed in to change notification settings - Fork 21
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
Remove some js/src/external-components
#643
base: develop
Are you sure you want to change the base?
Conversation
Does this have any implications for our support of older versions of wc/wc-admin/wp? |
Ha! Good question, I haven't thought about it :| Is there some kind of compatibility table between The README of the entire wc-admin, states
(for us it's "- WordPress 5.3+ - WooCommerce 4.5+" ) I did some digging and it seems that they bumped WP to |
I have WooCommerce 4.9.2 and WooCommerce Admin 2.12. in my local site. If the status of the wc-admin plugin is
and both of them have the |
Yep as you say, that version bump happened in Feb after we'd kicked things off. I don't think we can rush this one right now. Let's knock over the reporting issues and then circle back |
I guess, it got unlocked after we bumped required versions to
in #803 |
Unfortunately, WC |
@jconroy @eason9487 Do you know why we can't use our own version of I thought that was the main advantage of React over Web Components, that using different versions of the same component is easy and seamless. Is this a technical or policy limitation? |
I guess it's a consideration of the overall bundle size of js files. But actually, we're still using our own specific version of Also, if we can use our own specific dependency versions, there should be fewer compatibility issues with different versions of WordPress and WooCommerce. |
I'm rebasing this PR on top of #1110 as there we move to WC5.6+ => |
as those fixes were released in `@woocommerce/components`: - woocommerce/woocommerce-admin#6890, - woocommerce/woocommerce-admin#6062 Remove no longer needed dependencies.
dda400e
to
4924446
Compare
js/src/external-components
js/src/external-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.
Tested with WC 5.7 and 5.9, and there are no errors in either.
🚧
However, an inappropriate falsy check was later added to the <FilterPicker>
component, resulting in the free listings program ID 0
being cleared from the URL query programs
when filtering single program by free listings program.
Kapture.2021-12-03.at.13.28.38.mp4
Before: page=wc-admin&reportKey=programs&path=%2Fgoogle%2Freports&filter=single_program&programs=0
After: page=wc-admin&reportKey=programs&path=%2Fgoogle%2Freports&filter=single_program
One relevant hidden influence is that the isSubset
check here will not be true
, but in terms of results, it will still fall into the else
logic, making the correct rendering result of single program filter. It can be reproduced by refresh the page with path /wp-admin/admin.php?page=wc-admin&reportKey=programs&path=%2Fgoogle%2Freports&filter=single_program
.
google-listings-and-ads/js/src/reports/programs/filter-config.js
Lines 74 to 82 in 4924446
// If it's a subset of static values, resolve it immediately. | |
if ( isSubset( ids, freeListingsProgramsIds ) ) { | |
programs = freeListingsPrograms; | |
} else { | |
// If there are any paid programs, resolve it once we get it. | |
programs = ( await promiseProgramsList ).filter( ( campaign ) => | |
ids.has( campaign.id ) | |
); | |
} |
Another influence is that it still queries the APIs for paid programs and shows incorrect reporting results.
📜 Probably solution
Fix by WC-admin will need to continue waiting for the next L-2 to include the patch. To move forward with this PR, I tried changing FREE_LISTINGS_PROGRAM_ID
constant to -1
and it seems to work correctly, but I didn't check if there are new side effects afterward.
Thanks for finding that! This looks almost like a regression for woocommerce/woocommerce-admin#7028 I think it really unfortunate, we cannot use either the old or new version of |
Changes proposed in this Pull Request:
This is a cleanup task, as the fixes for forked components were released.
@woocommerce/components
to the latest6.2.0
from5.1.2
(7953722)ReportFilters
& co. (53ea575)as those fixes were released in
@woocommerce/components
:CompareFilter
does not forwardautocompleter
setting to theSearch
woocommerce-admin#6890,as
Search
fix Search component does not accept static options Array in autocomplete woocommerce-admin#6061 was released.Screenshots:
Detailed test instructions:
According to the woocommerce/components/CHANGELOG.md#620 the majority of changes were around
Search
,SummaryNumber
and<Filter>
components. So report pages are at the biggest risk, to be broken.Changelog Note:
Additional notes:
external-components
makes it easier for us to catch up with other dependencies. For example, the latest@woocommerce/eslint-plugin
complains a lot about external dependencies and experimental components being used. We have those problems also in our components, but this PR reduces a few.