Skip to content
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

Connect Programs filter to data source. #537

Merged
merged 4 commits into from
May 6, 2021
Merged

Conversation

tomalec
Copy link
Member

@tomalec tomalec commented Apr 30, 2021

Changes proposed in this Pull Request:

Create ProgramsReportFilters to use customized ReportsFilter, that will use Date, Programs, and
Compare filters consistently with other wc-admin analytics.
Provide custom autocompleter for programs.

Implements part of #242.

⚠️ Requires wc-admin fixes

Screenshots:

Animation of filter picker being used and populated with real programs data

Review hints:

Changes to our components are in the first two commits https://github.com/woocommerce/google-listings-and-ads/pull/537/files/16728a6465480a81cc5a599da67fe1fcbebfe931

The others are importing unreleased wc-admin fixes: https://github.com/woocommerce/google-listings-and-ads/pull/537/files/16728a6465480a81cc5a599da67fe1fcbebfe931..feaff0901cb3997a6bda7e99773741e205756208

Detailed test instructions:

  1. Go to Programs report page: https://gla1.test/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Freports%2Fprograms
  2. Use date and program filter (all, single, compare)
  3. Check that params in the address bar URL updates accordingly, and UI looks correct.
  4. Check that the autocomplete is populated with actual programs.

To be done in a separate PR:

  1. Fix initial state. When you refresh the page or visit URL for the first time with programs chosen, the labels are not shown. This seems to ba another wc-admin issue, as CompareFilter & FilterPicker seems to ignore changing getLabels function (it updates labels, only when params changes)

    Screencast that shows missing labels

  2. When "Freee Listings" are chosen as a "single program", their label is not rendered. Probably because FilterPicker considers the selected option with id==0 the lack of selection.
    Screencast showing the lack of label

Changelog Note:

tomalec added 2 commits April 30, 2021 17:28
Create `ProgramsReportFilters` to use customized `ReportsFilter`, that will use Date, Programs, and
Compare filters consistently with other wc-admin analytics.
Provide custom autocompleter for programs.

Implements part of #242.
@tomalec tomalec requested a review from a team April 30, 2021 15:52
@tomalec tomalec self-assigned this Apr 30, 2021
@jconroy
Copy link
Member

jconroy commented May 3, 2021

Requires wc-admin fixes

Hey @tomalec just confirming, are you saying we'll need a specific version of wc-admin (that hasn't been released yet?)

If so, and if I understand correctly, I think we'll need to probably come up with our own component with fixes so that we can support them now

@tomalec
Copy link
Member Author

tomalec commented May 3, 2021

Requires wc-admin fixes

Hey @tomalec just confirming, are you saying we'll need a specific version of wc-admin (that hasn't been released yet?)

Unfortunately, yes :|

WDYT, of adding the copies of those fixed component files to /js/src/wc-admin/components/(filters|filter-picker|compare-filter)/index.js so it would be easier to get rid of them, once the respective fixes are released?

tomalec added a commit that referenced this pull request May 3, 2021
tomalec added a commit that referenced this pull request May 3, 2021
@tomalec tomalec force-pushed the add/242-program-filter branch from a5c494e to ca95e7e Compare May 3, 2021 16:19
@tomalec
Copy link
Member Author

tomalec commented May 3, 2021

I added the clones to /js/src/external-components/woocommerce/*.

This adds some noise to this PR's diff, and @woocommerce/experimental dependency adds to build warnings, but I hope it's still reviewable.

npm run start warnings
WARNING in ./node_modules/@woocommerce/experimental/build-module/index.js 9:24-43
"export 'Navigation' (imported as 'NavigationComponent') was not found in '@wordpress/components'
 @ ./js/src/external-components/woocommerce/compare-filter/index.js
 @ ./js/src/external-components/woocommerce/filters/index.js
 @ ./js/src/reports/programs/programs-report-filters.js
 @ ./js/src/reports/programs/index.js
 @ ./js/src/reports/index.js
 @ ./js/src/index.js

WARNING in ./node_modules/@woocommerce/experimental/build-module/index.js 10:34-63
"export 'NavigationBackButton' (imported as 'NavigationBackButtonComponent') was not found in '@wordpress/components'
 @ ./js/src/external-components/woocommerce/compare-filter/index.js
 @ ./js/src/external-components/woocommerce/filters/index.js
 @ ./js/src/reports/programs/programs-report-filters.js
 @ ./js/src/reports/programs/index.js
 @ ./js/src/reports/index.js
 @ ./js/src/index.js

WARNING in ./node_modules/@woocommerce/experimental/build-module/index.js 11:29-53
"export 'NavigationGroup' (imported as 'NavigationGroupComponent') was not found in '@wordpress/components'
 @ ./js/src/external-components/woocommerce/compare-filter/index.js
 @ ./js/src/external-components/woocommerce/filters/index.js
 @ ./js/src/reports/programs/programs-report-filters.js
 @ ./js/src/reports/programs/index.js
 @ ./js/src/reports/index.js
 @ ./js/src/index.js

WARNING in ./node_modules/@woocommerce/experimental/build-module/index.js 13:28-51
"export 'NavigationItem' (imported as 'NavigationItemComponent') was not found in '@wordpress/components'
 @ ./js/src/external-components/woocommerce/compare-filter/index.js
 @ ./js/src/external-components/woocommerce/filters/index.js
 @ ./js/src/reports/programs/programs-report-filters.js
 @ ./js/src/reports/programs/index.js
 @ ./js/src/reports/index.js
 @ ./js/src/index.js

WARNING in ./node_modules/@woocommerce/experimental/build-module/index.js 12:28-51
"export 'NavigationMenu' (imported as 'NavigationMenuComponent') was not found in '@wordpress/components'
 @ ./js/src/external-components/woocommerce/compare-filter/index.js
 @ ./js/src/external-components/woocommerce/filters/index.js
 @ ./js/src/reports/programs/programs-report-filters.js
 @ ./js/src/reports/programs/index.js
 @ ./js/src/reports/index.js
 @ ./js/src/index.js

WARNING in ./node_modules/@woocommerce/experimental/build-module/index.js 14:18-31
"export 'Text' (imported as 'TextComponent') was not found in '@wordpress/components'
 @ ./js/src/external-components/woocommerce/compare-filter/index.js
 @ ./js/src/external-components/woocommerce/filters/index.js
 @ ./js/src/reports/programs/programs-report-filters.js
 @ ./js/src/reports/programs/index.js
 @ ./js/src/reports/index.js
 @ ./js/src/index.js

WARNING in ./node_modules/@woocommerce/experimental/build-module/index.js 10:67-101
"export '__experimentalNavigationBackButton' was not found in '@wordpress/components'
 @ ./js/src/external-components/woocommerce/compare-filter/index.js
 @ ./js/src/external-components/woocommerce/filters/index.js
 @ ./js/src/reports/programs/programs-report-filters.js
 @ ./js/src/reports/programs/index.js
 @ ./js/src/reports/index.js
 @ ./js/src/index.js

WARNING in ./node_modules/@woocommerce/experimental/build-module/index.js 15:21-32
"export 'useSlot' (imported as 'useSlotHook') was not found in '@wordpress/components'
 @ ./js/src/external-components/woocommerce/compare-filter/index.js
 @ ./js/src/external-components/woocommerce/filters/index.js
 @ ./js/src/reports/programs/programs-report-filters.js
 @ ./js/src/reports/programs/index.js
 @ ./js/src/reports/index.js
 @ ./js/src/index.js

@tomalec tomalec force-pushed the add/242-program-filter branch from ca95e7e to feaff09 Compare May 5, 2021 10:15
Copy link
Member

@eason9487 eason9487 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing well and LGTM.

@tomalec tomalec merged commit d101898 into trunk May 6, 2021
@tomalec tomalec deleted the add/242-program-filter branch May 6, 2021 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants