-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat: Add Receipt Page from ecommerce to MFE #189
base: master
Are you sure you want to change the base?
Conversation
41ca02e
to
8a88ebb
Compare
7152add
to
328ee6b
Compare
Codecov ReportBase: 48.78% // Head: 50.35% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #189 +/- ##
==========================================
+ Coverage 48.78% 50.35% +1.57%
==========================================
Files 15 22 +7
Lines 164 284 +120
Branches 28 66 +38
==========================================
+ Hits 80 143 +63
- Misses 79 130 +51
- Partials 5 11 +6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
870f807
to
0cc9490
Compare
46f483b
to
9d7d7b3
Compare
(junk comment, can delete) |
yield put(fetchOrderBegin()); | ||
const result = yield call(OrderApiService.getOrder, orderToFetch); | ||
yield put(fetchOrderSuccess(result)); | ||
} catch (error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ are there pieces that can move out of the try and into the else block, so it's clearer which bit we're targetting? Or could any of these fail separately/differently?
46b7926
to
7c16f3a
Compare
data-order-id={order.number} | ||
data-total-amount={order.total_before_discounts_incl_tax} | ||
data-product-ids={order.order_product_ids} | ||
// data-back-button="{{ disable_back_button | default:0 }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional comment - will remove soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some feedback. Also, curious, were you intending to have tests cover the placed where Codecov says there is no test coverage?
src/components/NotFoundPage.jsx
Outdated
@@ -10,7 +12,16 @@ export default function NotFoundPage() { | |||
defaultMessage="The page you're looking for is unavailable or there's an error in the URL. Please check the URL and try again." | |||
description="error message when a page does not exist" | |||
/> | |||
<span>{error}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If error
is optional (looks like it defaults to null
), we should avoid rendering the span
if there is no error
provided otherwise its an empty span
in the DOM that serves no purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This page will only render if there is error in fetching the order (in src/receipt/saga.js
). But I see your point, better to be safe than render an empty element.
Although now that I think about it, the actual error in this case is not very useful to the end learner? Following the original but I am likely going to remove it from here
className="order-history table-bordered" | ||
itemCount={this.props.orders.length} | ||
data={this.getTableData()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this.getTableData
above, it looks like the data is formatted to include JSX elements inside of the data array itself in order to customize how the displayed is rendered in the table cell. However, by formatting the data passed to the data
prop in DataTable
with JSX elements, it'd prevent the table from supporting client-side sorting/filtering/etc. since it no longer has access to the underlying data itself. DataTable
only has access to the JSX elements instead.
Alternatively, the proper way to format the cell data in DataTable
is by overriding the Cell
attribute within the columns
definition. See these docs as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done that in ReceiptPage.jsx
when I learned about Cell
from you 😬, I will update this here, good catch because I was just updating the deprecated table component to use DataTable
, thanks!
const { LMS_BASE_URL } = getConfig(); | ||
const DASHBOARD_URL = `${LMS_BASE_URL}/dashboard`; | ||
const FIND_COURSES_URL = `${LMS_BASE_URL}/courses`; | ||
class ReceiptPage extends React.Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[curious] Is the reason you're using a class component here because this is based on some other page component? Typically, we tend to prefer functional components over class components now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason is likely that I am rusty in React and still use class components 😅 For state I will useState()
hooks, assuming that's the way to go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should totally make the jump to functional React components 😉 It's refreshing haha!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yes, if you want to change this to a functional component, useState
is how you'd implement state, yes. That said, the decision on whether to change it from class -> functional is up to you :) I was just curious.
} | ||
|
||
.page__receipt { | ||
h2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[curious/confirmation] Are the styles here relevant for all h2 on the page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They should be, but weirdly I am modifying the size of 1 out of the 2 h2's with a specific px size.
I think this is another case of "following the original" which is not a good explanation but I will change them to be different heading levels since they are different.
} | ||
|
||
.thank-you { | ||
font-size: 38px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[curious] Why 38px here? It's not a font size in our design system's type ramp. See https://paragon-openedx.netlify.app/foundations/typography.
If we do need a custom font size here, we should use convert this to rem
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The aforementioned px specific size 😅 - will update to Paragon friendly size! Thanks for asking!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And preferably using a SCSS variable or CSS utility class, too (not hardcoded) 😉
|
||
.copy { | ||
> div { | ||
margin-bottom: 1rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, it's preferred to use Paragon's SCSS spacer or utility classes than hard coded rem
values for margins, e.g. mb-3
or map-get($spacers, 3)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this specifically, but sometimes I come across a case where I want a specific size that is not the same as bootstrap utility margin class spacer value, would I just revert to using a pixel or a fraction of a rem
here?
Why is it preferred to use utility classes, because they render with the component as opposed to being compiled in a separate SCSS file? Just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sometimes I come across a case where I want a specific size that is not the same as bootstrap utility margin class spacer value, would I just revert to using a pixel or a fraction of a rem here?
Personally, I find the closest existing spacer value to the pixel value I'm given. In an ideal world, the handoff from design -> engineering wouldn't refer to any hard coded pixel/rem values and would instead refer to an existing spacer value.
Why is it preferred to use utility classes, because they render with the component as opposed to being compiled in a separate SCSS file? Just curious.
If spacing throughout the UI is using the same spacer values, it's trivial to update our spacing definitions in once place and have it apply everywhere. If we only use hard coded values for spacing, and our foundation spacing design tokens change (e.g., maybe mb-3
is equivalent to 1rem
now, but 2rem
in the future), that change wouldn't propagate where spacing is hardcoded, leading to inconsistency in the UI.
} | ||
|
||
.order-total { | ||
font-size: 0.875rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the small
type ramp in our design system. This could be replaced with $font-size-sm
or use a small
utility class name on the element itself.
margin: 2rem 0 4rem 0; | ||
|
||
.dashboard-link { | ||
margin-right: 2.5rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense now that I understand ActionRow
better! Someone in fedex recommended this and I didn't stop to think that it needs an action by the name of the component (for the other case I used it for just text formatting)
…rder with try block
7c16f3a
to
1d13a50
Compare
REV-2683.
We're moving the Receipt Page from ecommerce repo to this MFE for the Theseus Project.
Current receipt page template: https://github.com/edx/ecommerce/blob/947c9068928281d24a54a86ea6df3340309a4f57/ecommerce/templates/edx/checkout/receipt.html#L1
Related backend PRs:
openedx/ecommerce#3748
openedx/ecommerce#3778
openedx/ecommerce#3781
Note: for screenshots of all the different versions, pls refer to https://2u-internal.atlassian.net/l/cp/N8fuc5Py
Receipt page before:
Receipt page after:
Order History update tag-along:
This is not part of this ticket, but Paragon's
Table
component will be deprecated, I'm updating the Order History page to useDataTable
so that both Order History and Receipt Page have the same look for tables.This PR:
DataTable
Paragon componentThis PR is dependent on the backend work in https://2u-internal.atlassian.net/browse/REV-2788
To test locally:
http://localhost:1996/receipt/?order_number={order-number}