Subscriptions Wizard#73
Conversation
claudiulodro
added
the
[Status] Needs Review
jeffersonrabb
left a comment
jeffersonrabb
left a comment
There was a problem hiding this comment.
Looks great! I left a few comments but all are suggestions, nitpicks, or questions which needn't block merging.
| /** | ||
| * Get the latest subscriptions info. | ||
| */ | ||
| refreshSubscriptions() { |
There was a problem hiding this comment.
[Suggestion] In a future PR we should establish a UI pattern for showing that an API call is in progress. Could be a Spinner, graying out the Wizard, etc. cc: @sonjaleix
| this.setState( { | ||
| subscriptions: subscriptions, | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
[Suggestion] For a future PR, it would be good to establish a pattern for catching and displaying API errors to the user.
| const { id, image, name, display_price, url } = subscription; | ||
|
|
||
| return ( | ||
| <Card className="newspack-manage-subscriptions-screen__subscription-card" key={ id }> |
There was a problem hiding this comment.
[Note] I'm planning to make a new component based on this UI that @sonjaleix designed, and when it's ready we can probably swap it in place of the Card here. I'll include support for the image to make this possible.
|
|
||
| return ( | ||
| <Card className="newspack-manage-subscriptions-screen__subscription-card" key={ id }> | ||
| <a href={ url } target="_blank"> |
There was a problem hiding this comment.
[Question] Why are the images links?
There was a problem hiding this comment.
Ideally we want to let people see the subscription on the frontend somehow. This is the best way IMO.
There was a problem hiding this comment.
I see - so these would eventually link to the /subscribe page?
There was a problem hiding this comment.
It links out to the purchase page for that specific membership.[1] People will be coming to this page from the /subscribe landing page to actually place their order.
[1] e.g. 
jeffersonrabb
added
[Status] Approved
|
Thanks for the thorough review and good suggestions. I've implemented the suggestions in d74b66f |
|
@claudiulodro This is looking great! The tooltip of the "name your price" checkbox runs pretty wide on my machine – as wide as the copy runs. I'm wondering if we should implement a max width. How is this handled in other places in the WP backend? We're also missing an "I'm done adding" link at the bottom as soon as the first subscription has been added to close the wizard. |
3 participants
All Submissions:
Changes proposed in this Pull Request:
Closes #53 .
Supersedes #15.
Supersedes #64 .
This implementation of the subscriptions wizard does so with all of the state pulled up into the main wizard class, as per @jeffersonrabb's suggestion. I think it was a good suggestion, and this seems easier to maintain and follow the code logic.
How to test the changes in this Pull Request:
npm ci; npm run clean; npm run build:webpackOther information: