Subscriptions Wizard by claudiulodro · Pull Request #64 · Automattic/newspack-plugin

claudiulodro · 2019-05-17T17:33:24Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Closes #53 .
Supersedes #15.

This is a rather large PR, however it lays the groundwork for all future wizards, so it should be straightforward after this.

Changes:

Moved the components demos into a new Components Demo wizard (this is mostly why the changed line count is so large)
Implement the Subscriptions Wizard
Some small bug fixed for issues blocking development of this feature (I've pointed these out in comments below)

One wizard has a simple pattern (in my opinion):

On the backend, each wizard maintains its own endpoints in the associated PHP Wizard class.
On the frontend, each screen is self-contained and manages its own data. The main wizard class exposes a changeScreen method to the screens to enable changing to different screens when buttons are clicked and stuff.

Not implemented in this initial version:

Loading UI (spinner, etc.) while waiting for API requests. We should discuss how we want loading to behave in general. There is a Muriel design for a loading spinner.
Plugin requirements handling. See Component: Required plugins #55. For now make sure you have WooCommerce, WooCommerce Subscriptions, and WooCommerce Name Your Price active.

How to test the changes in this Pull Request:

npm ci; npm run clean; npm run build:webpack.
Go to Newspack > Components Demo and verify that's still working.
Go to Newspack > Subscriptions.
Add and save subscriptions.
Test editing a subscription.
Test the "Allow members to specify donation amount" checkbox.
Click on the small, round image to see a subscription on the frontend.
Test deleting a subscription.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully ran tests with your changes locally?

claudiulodro · 2019-05-17T17:34:31Z

+		font-style: normal;
+		margin-top: 0;
+		color: $muriel-gray-600;
+	}


Bug fix: The help prop was unstyled for our checkboxes, causing it to go under the checkbox which looked ugly. I've added styling for it to make it look like our designs.

claudiulodro · 2019-05-17T17:36:25Z


 			return (
 				<BaseComponent
+					value={ value }


Bug fix: Since value was extracted from props above, it wasn't getting passed to BaseComponent, meaning you couldn't properly set a value on selects. I've not investigated whether the same issue happens for disabled, but it likely has the same problem. cc @iuravic

claudiulodro · 2019-05-17T17:37:06Z

 	}

-	input[type="text"] {
+	input[type="text"], input[type="number"] {


Bug fix: number inputs were not getting our styles.

claudiulodro · 2019-05-17T17:41:24Z

+		parent::__construct();
+
+		add_action( 'rest_api_init', [ $this, 'register_api_endpoints' ] );
+		add_filter( 'woocommerce_product_data_store_cpt_get_products_query', [ $this, 'handle_only_get_newspack_subscriptions_query' ], 10, 2 );


The Subscriptions Wizard only displays and manages products created through the Subscriptions Wizard. I think that's the way to go because it would be super confusing/breaking if we pulled in swag or other such products. This should keep things working nicely.

claudiulodro · 2019-05-17T17:43:56Z

 	<rule ref="PHPCompatibilityWP"/>
 	<config name="testVersion" value="5.6-"/>

-	<arg name="extensions" value="php,js"/>


Tweak: I've removed the js check from phpcs.xml because phpcs freaks out on React code and thinks that e.g. <div> is using greater/less-than operators, so it generates hundreds of style errors. Additionally, now that we have prettier working, we probably don't need this. We can re-approach after this is merged if we want to add it back.

philipjohn

I've not looked at the code yet but have loaded the branch up locally in order to test the user experience first of all, and have some feedback to share mainly on that.

First though, can we split the demo stuff out? I feel like it muddies this PR because it's not just the Subscriptions wizard. Can we instead split out the demo stuff into a separate branch to work on it there?

Also, a relatively minor point - it's best to be descriptive with branch names because something like add/53 means very little to someone coming in from outside, or in the future, and looking at commits. I like having the issue number and often used a format like add/[issue-no]_[description].

The "I'm done adding" link doesn't seem to do anything and feels unnecessary, can we do without it?

I added a name ("Understand") and amount ("12") and clicked "Save" but nothing happened. It looked like it may be because I didn't have pretty permalinks (something we need to catch) but even after applying them, I see this when trying to save:

POST http://newspack.test/wp-json/newspack/v1/wizard/subscriptions?_locale=user 500 (Internal Server Error)

It looks like that is triggered by this fatal:

PHP Fatal error: Uncaught Error: Class 'WC_Product_Subscription' not found in /srv/www/newspack/public_html/wp-content/plugins/newspack-plugin/includes/wizards/class-subscriptions-wizard.php:188

I have installed and activated, but not configured in any way, WooCommerce, WooCommerce Subscriptions and WooCommerce Name Your Price. Are we sure there isn't some extra setup set before that class is available?

I was also expecting the UI to be like the Membership Setup screen in Figma, that we currently have on the "Subscriptions" sub menu item on the master branch:

I won't dig into the code just yet.

claudiulodro · 2019-05-20T20:14:40Z

@philipjohn can you paste your system status report (WooCommerce > Status) so I can reproduce your environment?

I'll split out the PR into multiple ones tomorrow also; that should make it easier to review.

jeffersonrabb · 2019-05-21T09:49:14Z

I'll split out the PR into multiple ones tomorrow

Since this one might be broken into separate PRs I'll hold off on line-by-line review for now, but I'll note some broad architecture questions here. As context, I'm trying to take a long view and think about establishing SPA patterns that we can use for 100% of our wizards, so if any of these points seem like pedantic over-engineering, that's kind of where my head's at.

It seems to me we have a controller (index.js) and two views—a list (manageSubscriptionsScreen.js) and a detail (editSubscriptionScreen.js). If this is an accurate description of the structure, we probably want all state management and API interaction (essentially anything related to model) in the controller, and the views should be concerned with display concerns only. To achieve this, we would lift state from the views to the controller as described here https://reactjs.org/docs/lifting-state-up.html. All of the API interactions would be moved to index.js and the list/detail views would be stripped down to basically just a render() function and a few event handlers. The views would receive data as props and pass changes up to the controller via onChange events.
I'm also wondering if simple state will be sufficient for out needs, or if we might need to introduce Redux (https://redux.js.org/) as a state container? Per this, maybe not?
I don't think Components should be stored in state as you have here. Explanation: https://shripadk.github.io/react/docs/interactivity-and-dynamic-uis.html#what-shouldnt-go-in-state.
Thinking through ☝️ I was trying to figure out the best way to manage view hierarchies, and I wonder if we should be considering implementing a Router, e.g. https://github.com/ReactTraining/react-router?

Overall I found this doc useful: https://reactjs.org/docs/thinking-in-react.html

philipjohn · 2019-05-21T10:18:00Z

Will send you the status report via DM...

Just to check I actually have this right, I'm using these two plugins:

Also, I'm deliberately playing dumb and doing nothing to them and only interacting with the Newspack wizard.

philipjohn · 2019-05-24T14:55:01Z

I have installed and activated, but not configured in any way, WooCommerce, WooCommerce Subscriptions and WooCommerce Name Your Price. Are we sure there isn't some extra setup set before that class is available?

Just noting this was because I had the wrong plugins and that it's all working lovely for me with the correct plugins.

claudiulodro · 2019-05-24T17:02:44Z

See #73

claudiulodro added 12 commits May 14, 2019 11:03

Save progress

ad6afda

Bring up-to-date with master

7dc8842

Begin work on edit sub screen

9ced45e

Add state to edit screen

385157b

Sub creation/editing functionality

0c6a596

nyp functionality

f61bc1d

Delete functionality

c162fe5

Handling for when no products

e169e2b

Clean up code

9528b8a

Nice styling

fdbfd00

Fix help text on checkboxinput

1ec4e4d

Bring up-to-date with master

7fd30de

claudiulodro added the [Status] Needs Review The issue or pull request needs to be reviewed label May 17, 2019

claudiulodro commented May 17, 2019

View reviewed changes

philipjohn added the [Status] Under Review label May 20, 2019

philipjohn suggested changes May 20, 2019

View reviewed changes

philipjohn removed the [Status] Under Review label May 20, 2019

This was referenced May 21, 2019

Move components demo into its own wizard #66

Merged

Fix prop issues in select component #67

Merged

Style 'help' text for checkboxes #68

Merged

Style number/password/etc. inputs #70

Merged

Remove js from phpcs check #71

Merged

claudiulodro mentioned this pull request May 24, 2019

Subscriptions Wizard #73

Merged

5 tasks

claudiulodro closed this May 24, 2019

claudiulodro deleted the add/53 branch May 24, 2019 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscriptions Wizard#64

Subscriptions Wizard#64
claudiulodro wants to merge 12 commits into
masterfrom
add/53

claudiulodro commented May 17, 2019 •

edited

Loading

Uh oh!

claudiulodro May 17, 2019 •

edited

Loading

Uh oh!

claudiulodro May 17, 2019 •

edited

Loading

Uh oh!

claudiulodro May 17, 2019 •

edited

Loading

Uh oh!

claudiulodro May 17, 2019

Uh oh!

claudiulodro May 17, 2019

Uh oh!

philipjohn left a comment

Uh oh!

claudiulodro commented May 20, 2019

Uh oh!

jeffersonrabb commented May 21, 2019 •

edited

Loading

Uh oh!

philipjohn commented May 21, 2019

Uh oh!

philipjohn commented May 24, 2019

Uh oh!

claudiulodro commented May 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

claudiulodro commented May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

Uh oh!

claudiulodro May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claudiulodro May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claudiulodro May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claudiulodro May 17, 2019

Choose a reason for hiding this comment

Uh oh!

claudiulodro May 17, 2019

Choose a reason for hiding this comment

Uh oh!

philipjohn left a comment

Choose a reason for hiding this comment

Uh oh!

claudiulodro commented May 20, 2019

Uh oh!

jeffersonrabb commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philipjohn commented May 21, 2019

Uh oh!

philipjohn commented May 24, 2019

Uh oh!

claudiulodro commented May 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

claudiulodro commented May 17, 2019 •

edited

Loading

claudiulodro May 17, 2019 •

edited

Loading

claudiulodro May 17, 2019 •

edited

Loading

claudiulodro May 17, 2019 •

edited

Loading

jeffersonrabb commented May 21, 2019 •

edited

Loading