-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
✔️ Deploy Preview for basestore ready! 🔨 Explore the source changes: dd69d79 🔍 Inspect the deploy log: https://app.netlify.com/sites/basestore/deploys/622b92d1389edd0008da7931 😎 Browse the preview: https://deploy-preview-371--basestore.netlify.app |
Gatsby Cloud Build Reportbasestore 🎉 Your build was successful! See the Deploy preview here. Build Details🕐 Build time: 8m PerformanceLighthouse report
|
Preview is readyThis pull request generated a Preview👀 Preview: https://preview-371--base.preview.vtex.app |
Why this PR is not here @vtex-sites/storeframework.store ? |
Because I want to have the less difference between this repo and storeframework.store. Without the need for CMS, the |
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.
LGTM! Found no visual regression on the main pages and the final organization on pages/*
looks like what's expected (with only sections).
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.
Nice Job! 🎉
should we remove the Section
from the search page as well?
@@ -78,7 +78,7 @@ function GalleryPage({ | |||
/> | |||
<div className="product-listing__results-sponsored"> | |||
<h3>Sponsored</h3> | |||
<ProductTiles products={productsSponsored.slice(0, 2)} /> | |||
<ProductTiles products={productsSponsored.slice(0, 2)} title="" /> |
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.
Should we make the title prop optional?
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 sure. This component is weird because it's using another section. Sections should be self contained and not depend on other sections. This should be refactored on another PR. I'll add a TODO for this. Thanks!
Nice! I haven't seen that! I'll change it |
8d2fda9
to
508ef8e
Compare
What's the purpose of this pull request?
The whole idea of sections is to split app logic into replaceable/reusable strips of screen content so that CMS users can easily replace/rearrange the desired component on the screen. To allow CMS users to change the page's section, prismic and other CMSs usually use the following Render component logic:
This means that the
Section
map has to render a self contained section.This PR moves all external logic that were hard coded into the
/pages
components into the sections so CMS users can correctly change and re-order the sectionsHow to test it?
Nothing should have changed
Checklist