Add initial components and dependencies for the GA4 ecommerce demo#824
Add initial components and dependencies for the GA4 ecommerce demo#824ikuleshov merged 16 commits intogoogleanalytics:mainfrom
Conversation
… events generated by the demo app.
# Conflicts: # src/components/ga4/EnhancedEcommerce/store-context.tsx
| import * as React from "react" | ||
| import {StoreProvider} from "./src/components/ga4/EnhancedEcommerce/store-context" | ||
| import CustomLayout from "./gatsby/wrapRootElement.js" | ||
| import "./src/styles/ecommerce/variables.css" |
There was a problem hiding this comment.
Not strictly necessary, but it'd probably be better if styling was consistent accross this repo. Currently, the main form of styling is through react mui's useStyles hook (example)
There was a problem hiding this comment.
Thanks, didn't realize that. Looks like I need to move the css variables to Layout/useStyles.ts ? I'll probably do this in a separate PR then.
There was a problem hiding this comment.
Following up in separate PR sgtm.
There are a lot of styling options for mui. I usually recommend doing styles at the ./src/components/index.tsx level. You can define the hook inline, or pull it out to its own file. Kinda just depends on preference. One of the big things to keep in mind with mui styling is you don't have to worry about global namespcing because it generates unique class names based on your definitions. This makes it easier to have local css that is unique to the component it's working on.
| @@ -1,5 +1,6 @@ | |||
| src/images/* | |||
There was a problem hiding this comment.
Just to double check, this PR only adds in the components, but no new pages, correct? Just getting the repo to be ready for the demo in the future?
There was a problem hiding this comment.
Yes, just trying to keep the size of a PR reasonable.
| // Wrap every page using a StoreProvider object used by eCommerce demo. | ||
| export const wrapRootElement = ({ element }) => ( | ||
| <StoreProvider>{element}</StoreProvider> | ||
| ) No newline at end of file |
There was a problem hiding this comment.
nit: I think running prettier & yarn format --fix should clean up newlines, etc.
|
|
||
| // Wrap every page using a StoreProvider object used by eCommerce demo. | ||
| export const wrapRootElement = ({ element }) => ( | ||
| <StoreProvider>{element}</StoreProvider> |
There was a problem hiding this comment.
Is this store provider actually needed for all pages? My gut is that it'd be better to put this provider at the future src/pages/ecommerce page instead.
There was a problem hiding this comment.
Although I agree, I am yet unable to find another way to get the same instance of StoreProvider shared between different pages (e.g. product-index and product-details). If I move StoreProvider from the wrapper to, say, Layout component, each page gets its own instance of StoreProvider which obviously results in the cart being lost once you navigate between pages. I'll try to find a way to limit she scope of StoreProvider to the ecommerce demo only, but have not figured this out yet.
There was a problem hiding this comment.
Ah, I see.
If you have actual distinct pages (in ./src/pages/) this would be the only way to do this via useContext. That's probably fine, but another option that might be worth exploring is to manage that state in redux, which is already wired in via here. This isn't a silver bullet either, so I'm happy with whatever decision you come to on the tradeoffs of keeping it in different places.
There was a problem hiding this comment.
Oh one last thing to consider. Since this is already a firebase project, it should be fairly trivial to start using firestore if you want to persist cart data.
There was a problem hiding this comment.
True, but this is really just a demo. If we have an option of not storing any data on the server at all, I'd rather vouch for that...
2 participants
No description provided.