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

Create an empty Reports Products page, move Reports Programs files around #170

Merged
merged 4 commits into from
Jan 31, 2021

Conversation

tomalec
Copy link
Member

@tomalec tomalec commented Jan 26, 2021

This is a baby step for #97 and #98, that would help to develop those two independently.
It also divides the work, that most of renaming and moving around is separated here, and should not blur the picture on other PRs.

Changes proposed in this Pull Request:

  • Moves existing Program Reports code into /reports folders and namespace. (0254251)
  • Creates an empty /reports/products page. (96c9b11)
  • Remove unnecessary style import from MetricNumber - a leftover from Add Program Analytics page #109, which I discovered by the way of moving files around. (2ec329a)

Screenshots:

Screenshot of the empty Products Reports page

Detailed test instructions:

  1. Checkout this branch,
  2. npm install && composer install && npm run build
  3. Open http://localhost/wordpress/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Freports%2Fproducts (replace localhost/wordpress with your local env)

Changelog Note:

none

Additional comments

I have stopped before implementing the sub-navigation as required in #98, to:

Given, I'm not native English, I wasn't sure whether the page title should be "Reports Programs", "Programs Reports" or "Program Reports". I left it as it was in Figma's node title.

@tomalec tomalec requested a review from a team January 26, 2021 21:29
@tomalec tomalec changed the title Remove unnecesary style import from MetricNumber. Creates an empty Reports Products page, move Reports Programs files around. Jan 27, 2021
@tomalec tomalec changed the title Creates an empty Reports Products page, move Reports Programs files around. Create an empty Reports Products page, move Reports Programs files around Jan 27, 2021
@ecgan
Copy link
Member

ecgan commented Jan 28, 2021

The changes look pretty simple, looks good to me. Probably needs some PHP expert's eyes on the PHP code 😄

Given, I'm not native English, I wasn't sure whether the page title should be "Reports Programs", "Programs Reports" or "Program Reports". I left it as it was in Figma's node title.

I think the page title should be "Programs Report", as in "a report about programs" 🤔 .

@jconroy
Copy link
Member

jconroy commented Jan 29, 2021

I think the page title should be "Programs Report",

Yeah I think I'd agree - either "Programs Report" or just "Programs" and similarly "Products Report" or just "Products".

I look at as we have 2 reports - a "Programs report" and a "Products report".

Copy link
Member

@jconroy jconroy 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 for me, I think it just a couple of the title tweaked for consistency - similar to what @ecgan mentions

#170 (comment)

js/src/index.js Outdated
title: __( 'Reports', 'google-listings-and-ads' ),
container: Reports,
path: '/google/reports',
title: __( 'Programs Reports', 'google-listings-and-ads' ),
Copy link
Member

Choose a reason for hiding this comment

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

Programs Report for consistency below

js/src/index.js Outdated
],
__( 'Google Listings & Ads', 'google-listings-and-ads' ),
],
title: __( 'Products Reports', 'google-listings-and-ads' ),
Copy link
Member

Choose a reason for hiding this comment

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

Products Report for consistency below

@tomalec tomalec merged commit 507fe30 into trunk Jan 31, 2021
@tomalec tomalec deleted the feature/97_98-reports-pages branch January 31, 2021 22:19
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