-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add basket UI stuff (and a bunch of scaffolding) #10
Conversation
0015c5e
to
5276c89
Compare
mitodl/unified-ecommerce#183 is merged so this should be testable without having to do anything special really now (other than making sure your hostnames are correct). |
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.
All of the things you put in page-components
I would suggest putting in a new components
directory.
These are ... not named well ... and am definitely open to suggestions.
The intent was that page-components
are significant chunks of a page that might make API calls, are aware of application routes, etc.
Whereas components
is pure UI, no api calls. https://github.com/mitodl/unified-ecommerce-frontend/blob/main/docs/architecture/directory-structure.md
src/app/page.tsx
Outdated
<div> | ||
<h1>Home</h1> | ||
</div> | ||
<CartPageContainer> |
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 would use Container
from
import Container from "@mui/material/Container";
here. It is set up with breakpoints (specific to our theme) so stuff won't get wider than the screen.
src/app/page.tsx
Outdated
useEffect(() => { | ||
if (system && systems.data) { | ||
const foundSystem = systems.data.results.find( | ||
(integratedSystem: IntegratedSystem) => | ||
integratedSystem.slug === system, | ||
); | ||
|
||
if (foundSystem) { | ||
console.log("we found a system", foundSystem); | ||
setSelectedSystem(foundSystem.id); | ||
} | ||
} | ||
}, [system, systems, selectedSystem]); |
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 goal here is to set the value of setSelectedSystem
, and it's computed based on existing state. No need for useEffect
+ new state. Instead, just compute its value:
const systems = useMetaIntegratedSystemsList();
const selectedSystem = systems.data?.results.find(s => s.slug === system)
return (
{selectedSystem && <CartBody systemId={selectedSystem} />}
)
I believe this is the case for all useEffect
hooks in this PR: They all (1) add a new state variable to store XYZ, and (2) compute the value of XYZ based on existing state.
Request: In all those cases, get rid of the useEffect
and just compute the state.
Your useEffect
does have an advantage: it only runs when the relevant values change. 99% of the time that doesn't matter. If a computation is particularly expensive, or if equality-by-reference between renders is important, you can use useMemo
.
Edit: Pretty sure all other cases are in /cart/
which you said we'd get rid of.
}); | ||
}; | ||
|
||
const useDeferredPaymentsBasketRetrieve = (id: number, enabled: boolean) => { |
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 would get rid of this and make the one above
import type { UseQueryOptions } from "@tanstack/react-query"
const usePaymentsBasketRetrieve = (id: number, opts: opts: Omit<UseQueryOptions, "queryKey"> = {}) => {
return useQuery({
queryKey,
queryFn,
...opts
})
}
and just pass the opts where you need them.
options: PaymentsApiPaymentsBasketsListRequest, | ||
) => | ||
useQuery({ | ||
queryKey: ["paymentsBaskets"], |
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.
queryKey: ["paymentsBaskets"], | |
queryKey: ["paymentsBaskets", options], |
Anything that's used in the queryFn should go in the queryKey, otherwise
you make request with ?limit=10
later you make request with ?limit=20, and get cached value from earlier
}, | ||
}); | ||
|
||
const useDeferredPaymentsBasketList = ( |
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.
Similar, I would get rid of this and add opts to the other one.
.paymentsBasketsCreateFromProductCreate(slugAndSku) | ||
.then((response) => response.data), | ||
onSuccess: (_data) => { | ||
client.invalidateQueries({ queryKey: ["paymentsBaskets"] }); |
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.
Comment: If you change the query key above to ["paymentsBaskets", opts]
then this invalidation is still correct. Query keys invalidation is hierarararraarichal and (by default) match partially, so invaliding ["a", { x: 1}]
invalidates
["a", { x: 1}]
["a", { x: 1}, "b"]
["a", { x: 1, y: 2, z: 3}]
- but NOT
["a", { x: 100}]
or["a"]
.
Works well 👍 Left a few minor comments and simple requests. |
8d20d18 should address the comments here - this also removes the |
|
||
const usePaymentsBasketList = ( | ||
options: PaymentsApiPaymentsBasketsListRequest, | ||
opts: Omit<UseQueryOptions, "queryKey"> = {}, |
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.
My suggestion here was a little bit bad... it necessitated the typecasting you did elsewhere. What happened is: Because now TS thinks opts
might contain queryFn
, it can no longer infer the correct shape of the data.
If you do:
type ExtraQueryOptions = Omit<UseQueryOptions, "queryKey" | "queryFn"> = {}
const usePaymentsBasketList = (
options: PaymentsApiPaymentsBasketsListRequest,
opts: ExtraQueryOptions = {
) => {...}
all will be well.
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.
FWIW - I did this and it was still pretty confused, so the additional casting was left in place.
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.
Ah. I guess I've done
type ExtraQueryOptions = Pick<UseQueryOptions, "enabled" | ...any_others_we_care>
in the past. Some of the more complicated options do see to mess with TS.
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.
@jkachel As mentioned in slack, I somehow missed the place order button last week. I left a few requests. I know the UI is very unfinal, so if anything seems to ui-related, feel free to skip it. Comments I thought that definitely were worth doing I put "Request" next to.
|
||
const form = document.createElement("form"); | ||
form.method = "POST"; | ||
form.action = checkout.url; |
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.
Where should this go? I got http://ue.odl.localhost:8072/sample-setting
. Should it have gone to something related to test-system-1
? Or am I missing some backend setting?
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 should send you to CyberSource. If you're getting to that URL you probably have some configuration settings that need to be set in the Django app.
That said: merely progressing to the checkout page is a success for this; the frontend UI stops once you've hit the Place Order button. You're sent to the integrated system once you've completed payment. But, I can share with you some usable CyberSource settings if you want to do a complete test (just ping me on Slack and we'll work it out).
const handleClick = async () => { | ||
await checkoutMutation.mutateAsync({ system_slug: systemSlug }); | ||
|
||
if (checkoutMutation.isSuccess) { |
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.
Request: Get rid of the conditional... it's always false.
Generally state and props in react are immutable, so when state from usePaymentsCheckoutStartCheckout
changes (like isSuccess
) the hook returns a shallow copy of the data
// First render (First call of PlaceOrderButton)
const checkoutMutation = usePaymentsCheckoutStartCheckout()
const handleClick = ( ) => {...}
// then you click "Place order"
// API call finishes, `useMutation` updates its internal state
// causes a render (new call to PlaceOrderButton)
const checkoutMutation = usePaymentsCheckoutStartCheckout()
const handleClick = ( ) => {...}
But the event handler that fired when you click is the first render's version of handleClick
, so its closure has the first reference to checkoutMutation, which means checkoutMutation.isSuccess
is false.
Suggestion: You don't need to check checkoutMutation.isSuccess
at all, though. await Promise<...>
will throw an error if it fails, so if you ever get past await checkoutMutation.mutateAsync(...)
, then you know it was successful. And mutateAsync returns the data:
const checkoutData = await checkoutMutation.mutateAsync({ system_slug: systemSlug });
}; | ||
|
||
return ( | ||
<CartSummaryDiscountContainer> |
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.
Suggestion: bump smoot design and... smoot-design
v1.1.1 has TextField
added, which will handle a lot of the UI here for you. Sorry I didn't publish that version till just now.
So, I suggest changing the return value to:
<CartSummaryDiscountContainer>
<TextField
name="discountcode"
label="Coupon Code"
value={discountCode}
onChange={hndUpdateCode}
error={discountMutation.isError}
helpText={discountMutation.isError ? "Invalid discount code" : ""}
/>
<Button variant="secondary" onClick={hndApplyDiscount}>
Apply
</Button>
</CartSummaryDiscountContainer>
BUT: This needs CartSummaryDiscountContainer
to be:
const CartSummaryDiscountContainer = styled.div`
margin-top: 20px;
display: flex;
align-items: end;
justify-content: space-between;
`;
systemSlug, | ||
}) => { | ||
const discountMutation = usePaymentsBasketAddDiscount(); | ||
const [discountCode, setDiscountCode] = React.useState<string>(""); |
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.
Comment: No need for <string>
, TS will infer the correct type from ""
.
})); | ||
|
||
const CartSummaryTotalContainer = styled.div` | ||
${{ ...theme.typography.h5 }}, |
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.
Comma here (and a few other places) should be semi-colon if you're doing the styled.div...css..
. Can also do styled.div({ ...object...})
then its a comma, since js object.
I would avoid creating a theme (const theme = createTheme()
) for this. It's possible it's different from the actual theme that's passed into ThemeProvider
in providers.tsx
You could import the theme from src/components/theme
. But I'd just do:
const CartSummaryTotalContainer = styled.div`
${({ theme }) => ({ ...theme.typography.h5 })};
Or lately I've started preferring the object syntax:
const CartSummaryTotalContainer = styled.div(({theme}) => {
...theme.typography.h5,
marginBottom: 20px,
marginTop: 8px
})
because i feel it makes accessing the theme a little easier.
${{ ...theme.typography.body1 }}, | ||
color: ${theme.custom.colors.silverGrayLight}, |
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.
Request: commas should be semi-colons here and above. Will see if a linter can pick it up.
…omponents - Updates the OpenAPI client (which is just hard-set in here for now) to the latest version from the backend - Update the cart page to actually pull the cart, in a sorta convoluted manner - Add hooks for meta and payments (just what was in Learn, plus a couple of deferrable ones) - Add a utility function to get the system out of the URL (which probably needs refactoring to use the NextJS way of doing this) - Update EnsureSession to better format the redirect URL into the app - this is system dependent There are some backend changes that are necessary for some of this to work - so this can't be tested as is.
…asic chooser into the index route This is all pretty rough - mostly committing because it's end-of-day.
- The root page now has a chooser and a cart, and some logic to determine where you go. - Side-ported the Card component in and made some changes to make it work within the UE framework - Copied over pxToRem too - Got the bones set back up for the cart page Doesn't yet pull cart data, but that's arguably the easy thing. Some of the stuff in here should really go into smoot-design but my plan is to get them roughed in here first and debugged to an extent, then throw them in there (so I'm not waiting on reviews over there so much).
…ms display, update API client
…t really there yet and this doesn't allow you to checkout but it mostly looks right
- Gather stuff for each functional unit into its own file - DRY up the StyledCard (should probably rename this but it's ok for now) - Update the index page to use the new components and such We still don't post things but the APIs aren't exposed properly, so have to fix that first (in the Django app).
- Update the axios config to includ CSRF stuff - Update the generated API client - Add logic to apply the discount code - Add discount code display in the cart summary
- Docs updated wrt hostnames - this and the backend really need to be on the same origin or CSRF stuff gets wonky - Removed ListCard - having some weird issues with a type import in smoot-design but it's not being used right now so - Removed StoryBook stories (these don't belong here)
…etup to include the DOM stuff
- Move a bunch of layout components into `components` from `page-components` - Removed the cart and checkout pages (all that functionality should live in the root page) - Fixed typing and removed dependence on useEffect and useState for the root page - Removed the explicit deffered hooks in favor of allowing options to be specified for the regular query hooks
- Fixed some places where theme is used (both accessing the theme itself and importing stuff from the theme) - Fixed some issues where styling blocks weren't formatted correctly - Updated smoot-design and updated the discount field to use its TextField instead - Updated logic that handles the checkout form creation
3965a3e
to
4945a6d
Compare
You will need the backend changes from mitodl/unified-ecommerce#183 for this code to work.
What are the relevant tickets?
https://github.com/mitodl/hq/issues/5656
Description (What does it do?)
Adds two pages to the front end:
This also adds a bunch of scaffolding stuff. I've pulled a handful of components from
ol-components
in Learn into this - these need to be moved to smoot-design at some point but for the purposes of getting this to a state that people can see it they're just here for now. Similarly, the OpenAPI client is included while we work out the remaining issues with the pipeline build.This also has a "force reauth" that allows you to force the app to re-do the
establish_session
call. This is for debugging only.Screenshots (if appropriate):
Desktop screenshots only:
Chooser page:
Basket page:
Empty basket page:
How can this be tested?
You'll need a working UE backend setup. Make sure you have data in it - run
generate_test_data
if you haven't already - and for best results you should also have some tax rates set up and some discounts.Build and run the app.
To add items to a specific cart, use the test mule app:
http://ue.odl.local:9080/cart/<system slug>/
. You can also hit the API for this manually -/api/v0/payments/baskets/create_from_product/{system_slug}/{sku}/
.Additional Context
The goal for this PR (and its related PR in
unified-ecommerce
) is to get something up and running so we can make sure the deployments work and so that people can more easily see progress in the system. Because of that, there's some shortcuts that were taken and some things aren't completely baked yet. Notably:Input
component is using theInputBase
- this is already a WIP in smoot-design so opted to do a minimal amount here until it's ready.Also, an important note for local deployment: this and the API need to live on the same(ish) hostnames. So, you should access this from
ue.odl.local:8072
and the API should beue.odl.local:9080
. (There may be some references to the front end being atuefe.odl.local
- this won't share an Origin with the API and you'll get CSRF and CORS errors.)