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

Use "Completed Order" component in Profile page #3112

Merged
merged 15 commits into from
Nov 2, 2017

Conversation

efalayi
Copy link
Contributor

@efalayi efalayi commented Oct 15, 2017

Resolves #3018
Displayed orders list on user profile page uses the old completed order Blaze template but we should reuse the Completed Order component that was converted to React and made marketplace-aware.

Fix
  • Create a template for userOrdersList to manage list display
  • Create an helper function for userOrdersList template that return the CompletedOrder react component
  • Create a container component for userOrdersList
  • Fix CSS style errors
  • Update profile template with new template
Test
Login as admin
Create an order
Navigate to user profile page
Scroll to displayed user order
Observe that displayed list matches the completedOrder template

- Create a template for userOrdersList
- Create an helper function for userOrdersList
- Fix CSS style errors
- Update profile template with new template
@brent-hoover
Copy link
Collaborator

@efalayi As I've mentioned before, when we want Ryan to review something let's add the design-review label, not add him as a reviewer.

@brent-hoover brent-hoover removed the request for review from rymorgan October 16, 2017 01:06
@brent-hoover
Copy link
Collaborator

So you will need to make some adjustments to the completed order component because some of those elements don't make sense within the context of the profile page, e.g. the giant "Thank you".

account_profile

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Let's make sure that everything we display makes sense within the context of the user profile.

import CompletedOrder from "/imports/plugins/core/checkout/client/components/completedOrder";

/**
* @method handleDisplayMedia
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't there a way we could be rendering the Completed Order container rather than duplicating all this code?

<span data-i18n="cartCompleted.noOrdersFound">No orders found.</span>
</div>
{{/each}}
</template>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing CR/LF

Esther Falayi added 2 commits October 16, 2017 12:28
- Refactor userOrdersList helper function
@rymorgan
Copy link
Contributor

rymorgan commented Oct 16, 2017

Two things:

  1. Let's tweak the layout so that the items and cart portion stretch across the full box
  2. Also, on the user profile, the title should change from 'Your cart' to 'Order summary'

complete orders view - user profile

@efalayi
Copy link
Contributor Author

efalayi commented Oct 16, 2017

@ryan should the tweak in layout be specific to just the user profile page?

@rymorgan
Copy link
Contributor

@efalayi Yep, only on the user profile page.

@efalayi
Copy link
Contributor Author

efalayi commented Oct 17, 2017

@ryan I just observed that the layout above is a bit different from the one here: https://scene.zeplin.io/project/58af8900737c137280322bd6/screen/59b7272087fcafc969870826. Do we still intend to use the layout on zeplin?

@rymorgan
Copy link
Contributor

@efalayi The layout in Zeplin is where we are going in the future w/ a refreshed design of our user profile views. The issue we are working in this PR is using the component consistently. So use the designs I provided here vs the Zeplin ones, which we will tackle when we work on the rest of the user profile updates.

@efalayi efalayi changed the title Use "Completed Order" component in Profile page [WIP] Use "Completed Order" component in Profile page Oct 18, 2017
- Fix CSS styles on profile page
@efalayi
Copy link
Contributor Author

efalayi commented Oct 18, 2017

@zenweasel @rymorgan I've implemented all requested changes on this PR.

@efalayi efalayi changed the base branch from release-1.5.1 to master October 18, 2017 11:47
@rymorgan
Copy link
Contributor

rymorgan commented Oct 18, 2017

@efalayi I'm seeing some formatting problems and after that, this PR should be good from a design perspective.

Need to vertically center align the copy in the rows for 'Your items'.

account_profile

Need to fix casing
account_profile-2

On mobile -
You can reduce the padding by a half.
Stack shipping address and payment method.
Make sure the containers are all centered. (email/password, your orders, ad address) and not getting cut off.

iphone_x_-_ios_11_0

Esther Falayi added 2 commits October 19, 2017 09:36
- Aligns items properly
- Make titles sentence case
@efalayi
Copy link
Contributor Author

efalayi commented Oct 19, 2017

@rymorgan I've updated the PR with the changes specified.

@rymorgan
Copy link
Contributor

rymorgan commented Oct 19, 2017

A few of my comments didn't get addressed yet.

Match the spacing between outer box and inner box. The 'Your orders' box should match the address box. So, we need to reduce the spacing to match.

iphone_x_-_ios_11_0_and_inbox_ _morgan_ry_gmail_com

I know this wasn't in the original ticket, but while we are on this page, I'd like to fix the casing issue on the address book title. Should read 'Address book'.

account_profile-4

I also noticed two other spacing issues see below:

The spacing is inconsistent between all of the boxes. All the boxes w/ title should have even spacing between them. The Your items box has more space both above and below than it should. This is happening on mobile and desktop.

account_profile-5

There's an awkward unevenness between the shipping and payment boxes on desktop.

account_profile-3

- Fix element margin and padding
- Create list component for orders
- Refactor code
- Add JSDoc
@efalayi
Copy link
Contributor Author

efalayi commented Oct 21, 2017

@zenweasel I created an OrdersList component as you suggested and refactored the code. @rymorgan the css issues have also been fixed.

@efalayi efalayi changed the title [WIP] Use "Completed Order" component in Profile page Use "Completed Order" component in Profile page Oct 23, 2017
@brent-hoover
Copy link
Collaborator

@rymorgan Are we will saying "Your cart" in the completed order page? Should we just say "Your items"?

cart_completed

@brent-hoover
Copy link
Collaborator

@efalayi I spoke with @rymorgan and we agreed we should make it "Your items" (rather than You cart) on the completed order page as well as the profile page. Also we want to add Order ID on the Account version. Ryan has updated the comp.

@rymorgan
Copy link
Contributor

rymorgan commented Oct 24, 2017

Here's the profile comp with the order id.

complete orders view - user profile

@efalayi
Copy link
Contributor Author

efalayi commented Oct 24, 2017

I'll get to it and update the PR asap.

@efalayi
Copy link
Contributor Author

efalayi commented Oct 24, 2017

@zenweasel @rymorgan after making the change, this is what completed order page looks like:

completed-order-page

Your items is repeated twice. I'm not sure what the first does though.

@rymorgan
Copy link
Contributor

31971759-1f9edd8e-b915-11e7-9bac-4a59b87252ba_png__2560x1450_

- Fix error display
@efalayi
Copy link
Contributor Author

efalayi commented Oct 25, 2017

@zenweasel I just updated the PR with the changes. I noticed that no message was displayed if the user has no orders (that's now fixed in the update).

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

A few code changes here still. Mostly want to try and eliminate as much Blaze code as we can.

* @since 1.5.1
* @return {Array} - an array of orders
*/
function getOrders() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused as to why we are doing anything at all in the template. If we still need the template at all the only thing it should be doing is rendering the component.

{{> React completedOrders }}
</div>
{{else}}
<div class="alert alert-info">
Copy link
Collaborator

Choose a reason for hiding this comment

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

The component should be handling both the found and not found cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I'll create functions to render found and not found cases in the component.

return (
<div className="container order-completed">

let headerText;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This component should not need to be aware of what route it is in. It should be handled by passing in a prop. Components should be as dumb as possible.

@@ -10,16 +9,24 @@ import CompletedOrderItem from "./completedOrderItem";
* @property {String} shopName - The name of the shop
* @property {Array} items - an array of individual items for this shop
* @property {Function} handleDisplayMedia - A function for displaying product images
* @property {boolean} isProfilePage - Checks if current page is Profile Page
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consistent casing for Javascript types

return (
<div className="order-details-shop-breakdown">
{/* This is the left side / main content */}
<div className="order-details-info-box">
<div className="store-detail-box">
<span className="order-details-store-title">{shopName}</span>
<span className="order-details-shipping-name">{shippingMethod.carrier} - {shippingMethod.label}</span>
<span className="order-details-shipping-name">{shippingName}</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shippingName varies depending on whether the current page is profile page. It is defined in line 16.

@@ -6,9 +6,11 @@ import { Components, registerComponent } from "@reactioncommerce/reaction-compon
* @summary Displays the order summary for each shop
* @param {Object} props - React PropTypes
* @property {Object} shopSummary - An object representing the summary information for this Shop
* @property {boolean} isProfilePage - Checks if current page is Profile Page
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consistent casing

@brent-hoover
Copy link
Collaborator

Still not exactly right on mobile devices

iphone_x_-_ios_11_0_and_applications_and_cart_completed

@brent-hoover
Copy link
Collaborator

Seems like this order break could be delineated better. Really gets lost on mobile.

account_profile

@rymorgan
Copy link
Contributor

rymorgan commented Oct 31, 2017

@zenweasel Agree, let's add a rule between different orders. Two things:

  1. The orders ID text should be 16px not 14px font. Right now it's smaller than everything else.
  2. Let's add a rule between the orders. The rule should not extend across the whole container. It should be the width of the order box. Here's a mock.

orders detail

@efalayi
Copy link
Contributor Author

efalayi commented Nov 1, 2017

@zenweasel at what screen size does this occur?

iphone_x_-_ios_11_0_and_applications_and_cart_completed

I just tested with chrome tools.

- Fix css styles
- Fix typo errors
@brent-hoover brent-hoover changed the base branch from master to release-1.5.6 November 2, 2017 21:39
@brent-hoover brent-hoover merged commit aa2a1ea into release-1.5.6 Nov 2, 2017
@brent-hoover brent-hoover deleted the update-esther-fix-issue-3018 branch November 2, 2017 21:42
@brent-hoover brent-hoover mentioned this pull request Nov 2, 2017
Akarshit pushed a commit that referenced this pull request Jan 7, 2018
…e-3018

Use "Completed Order" component in Profile page
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