Adds base infra for the Unified Ecommerce frontend #1634
Conversation
- Added a "space" for this - EcommercePages - Added a CartPage placeholder (it's the Privacy Policy for now, just needed _something_) - Added a PostHogRoute component to allow us to feature flag the ecomm stuff (but may go a different route here, not sure) - Added routes for the cart page For this to work you'll need to make a feature flag called "enable-ecommerce" and turn it on in PostHog. (And you'll need to fix my code because this actually doesn't work right now.)
- Removed the PostHogRoute component as it didn't really work consistently - Added a EcommerceFeature component to do the feature flag checking in a standardized way - Removed a bunch of stuff from the "Cart" Page (since it was just the Privacy Policy anyway) Not using the PostHogFeature component here - I was trying, but I kept running into issues where it'd throw to the fallback if the flag was enabled. It seems much more stable now with the useFeatureFlagEnabled but I wouldn't be opposed to doing it manually, since this doesn't trigger the Feature Flag View event (and then we can also just.. wait for the flag to come back as something).
jkachel
added
Needs Review
| import { useFeatureFlagEnabled } from "posthog-js/react" | ||
|
|
||
| jest.mock("posthog-js/react") | ||
| const mockedUseFatureFlagEnabled = jest.mocked(useFeatureFlagEnabled) |
There was a problem hiding this comment.
| const mockedUseFatureFlagEnabled = jest.mocked(useFeatureFlagEnabled) | |
| const mockedUseFeatureFlagEnabled = jest.mocked(useFeatureFlagEnabled) |
There was a problem hiding this comment.
Minor Suggestion: I wouldn't bother with most of the styling here:
- A lot of it isn't doing anything, really. (e.g., the
align-self: stetchand various flexboxes yield the same layout as defaultdisplay: blockhere. - Some of this, like making the paragraph have
component="h2"is undesirable (not a header) and might be accidentally left in later.
Clearly this is all placeholder, so it doesn't really matter, but
<EcommerceFeature>
<Container>
<MetaTags title="Shopping Cart" />
<Breadcrumbs
variant="light"
ancestors={[{ href: urls.HOME, label: "Home" }]}
current="Shopping Cart"
/>
<Typography component="h1" variant="h3">
Shopping Cart
</Typography>
<Typography>
The shopping cart layout should go here, if you're allowed to see
this.
</Typography>
</Container>
</EcommerceFeature>looks fine, IMO
| throw new ForbiddenError("Not enabled.") | ||
| } | ||
|
|
||
| return <>{ecommFlag ? children : null}</> |
There was a problem hiding this comment.
No need for a fragment here.
| return <>{ecommFlag ? children : null}</> | |
| return ecommFlag ? children : null |
| */ | ||
|
|
||
| const EcommerceFeature: React.FC<EcommerceFeatureProps> = ({ children }) => { | ||
| const ecommFlag = useFeatureFlagEnabled("enable-ecommerce") |
There was a problem hiding this comment.
Might be worth putting these as constants in @/common/feature_flags or something.
(Or a TS enum, though they are occasionally annoying.)
|
This looks good. Left some very minor requests. Mostly the typo + unnecessary fragment. |
ChristopherChudzicki
added
Waiting on author
and removed
Needs Review
- Added an enum for feature flags - Updated places where I'd directly referenced the ecommerce flag to use the enum value instead - Stripped out a bunch of styling components from CartPage - this will all go away at some point anyway so why not now?
|
3a02e22 should address the remaining stuff. Also - it's probably not a bad idea to make the |
ChristopherChudzicki
left a comment
ChristopherChudzicki
left a comment
There was a problem hiding this comment.
👍 Also out of scope for this PR, but might want to make the posthog events an enum / constants, too.
| if (ecommFlag === false) { | ||
| throw new ForbiddenError("Not enabled.") | ||
| } |
There was a problem hiding this comment.
Post-merge comment
I noticed some odd behavior with PostHog and feature flags that I thought was worth mentioning. Essentially, I think there's no reliable way—with useFeatureFlagEnabled—to determine whether a flag is "off" or "not yet loaded". (Note: the useFeatureFlagEnabled is really simple. Here's its source.)
useFeatureFlagEnabled returns boolean | undefined. Its behavior is:
useFeatureFlagEnabled("flag-A")returns undefined before any flags have loaded- When any flags have loaded,
useFeatureFlagEnabled("flag-A")returnstrueif the flag is on, andfalseif the flag is off or has not loaded yet.- "When any flags have loaded" includes:
- bootstrapped flags
- flags determined via API call to posthog site
- "When any flags have loaded" includes:
So the following is possible:
In this scenario, we were attempting to:
- If flags not loaded yet, return null
- If flags loaded, either
- redirect
- or render the cart page
But this doesn't seem to work quite right if any flags are bootstrapped, as shown in the sreenshot.
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/5611
Description (What does it do?)
We're going to develop the frontend for Unified Ecommerce in Learn, and this adds some necessary support stuff to that:
EcommercePagesfolder so we can store the page components for the UE frontendEcommerceFeaturecomponent so we can easily feature flag out the UE stuff (especially now, since we'll be doing things to begin with that don't have design yet)CartPageplaceholder page, just so there's something to go to/cart/to go to the UE code - this requires a logged-in session and an enabled feature flag to accessHow can this be tested?
Automated tests should pass.
To manually test, you'll need to make sure your instance is hitting PostHog - so it needs a valid set of keys. You can either hit the "MIT Learn Test" project that's in the OL PostHog account, or you can spin up your own PostHog account, or you can spin up your own instance of PostHog locally; any of these will work. Then, you'll need to go into Feature Flags and create a flag that just releases a boolean value called
enable-ecommerce.Test by loading
<site root>/cart/:Additional Context
This doesn't add the API client for UE - we'll do that in a separate PR because it'll probably require some tweaking in the UE code. So, we're just focusing on getting UE some space set up to move into.