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

Refactor the product detail page and the cart #2132

Merged
merged 6 commits into from
Jun 20, 2024
Merged

Conversation

blittle
Copy link
Contributor

@blittle blittle commented May 20, 2024

We want to refactor the starter template, moving things around while maintaining existing functionality. The hope is to make the code more self documenting and easy to understand.

Things to include

  • Refactor the PDP
  • Refactor the cart
  • Add an extra component layer for deferred data
  • Add READMEs

image
image

Copy link
Contributor

shopify bot commented May 20, 2024

Oxygen deployed a preview of your bl-components-refactor branch. Details:

Storefront Status Preview link Deployment details Last update (UTC)
third-party-queries-caching ✅ Successful (Logs) Preview deployment Inspect deployment June 17, 2024 4:53 PM
custom-cart-method ✅ Successful (Logs) Preview deployment Inspect deployment June 17, 2024 4:53 PM
classic-remix ✅ Successful (Logs) Preview deployment Inspect deployment June 17, 2024 4:54 PM
metaobjects ✅ Successful (Logs) Preview deployment Inspect deployment June 17, 2024 4:53 PM
optimistic-cart-ui ✅ Successful (Logs) Preview deployment Inspect deployment June 17, 2024 4:54 PM
Skeleton (skeleton.hydrogen.shop) ✅ Successful (Logs) Preview deployment Inspect deployment June 17, 2024 4:54 PM

Learn more about Hydrogen's GitHub integration.

@blittle blittle force-pushed the bl-components-refactor branch from c016743 to f6df8a9 Compare June 12, 2024 17:36
@blittle blittle marked this pull request as ready for review June 12, 2024 18:12

This comment has been minimized.

@blittle blittle force-pushed the bl-components-refactor branch from 6f40686 to d821a68 Compare June 17, 2024 15:35
@@ -8,7 +8,7 @@ import type {
import {Aside} from '~/components/Aside';
import {Footer} from '~/components/Footer';
import {Header, HeaderMenu} from '~/components/Header';
import {CartMain} from '~/components/Cart';
import {CartMain} from '~/components/CartMain';
Copy link
Member

Choose a reason for hiding this comment

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

I don't love the name CartMain — but not sure what else it should be

Copy link
Member

@benjaminsehl benjaminsehl left a comment

Choose a reason for hiding this comment

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

overall nice changes — it's nice how much simpler it is, I really like how much less indirection there is on the PDP especially — and then componentizing cart can in the future be quite valuable

@blittle blittle merged commit 6a6278b into main Jun 20, 2024
13 checks passed
@blittle blittle deleted the bl-components-refactor branch June 20, 2024 18:05
@@ -309,6 +309,7 @@ export function PredictiveSearchForm({
{...props}
className={className}
onSubmit={(event) => {
debugger;
Copy link
Contributor

Choose a reason for hiding this comment

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

@blittle We probably don't want this 😅

@@ -93,7 +93,7 @@ export default function Cart() {
errorElement={<div>An error occurred</div>}
>
{(cart) => {
return <CartMain layout="page" cart={cart} />;
return <CartMain layout="page" cart={cart!} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should make CartMain get cart: Cart | null instead of adding ! everywhere?

Copy link
Member

Choose a reason for hiding this comment

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

but what if Bret just really wants to put a lot of emphasis on cart? ;)

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