Single Product Block #2399
Single Product Block #2399
Conversation
mikejolley
added
the
type: new block
|
Size Change: +131 kB (6%) 🔍 Total Size: 2.13 MB
ℹ️ View Unchanged
|
|
@mikejolley how would you feel about putting the bare minimum in place for this block and merging in behind a feature flag? That way we can avoid a long running development branch and keep pull requests relatively lean? |
|
@nerrad given the amount of focuses this cycle I want to keep PR reviews down; so one review/test of the block itself. Supporting changes (e.g. API) I will raise separately and rebase. There isn't much point in putting an incomplete block behind a flag imo. I think those have more value for beta testing? |
There's a few reasons why I prefer the feature flag approach:
About the only benefit I see for having a feature branch is to keep that work isolated from the rest of the code and prevent prematurely exposing to users. I think the latter is solved nicely by the feature flag and with the former I don't think that's something we need to worry about (and actually I'd sooner know about impacts to other existing code earlier in the development cycle rather than later after we merge the whole caboodle sometime down the road. I'm definitely all ears 👂 if you have some reasons for keeping things in a feature branch that I haven't considered? |
I'm not doing that. I'm using this branch to get the block working and to a testable, reviewable state. I'm happy to put it behind a flag at that point, but not before. I mentioned the focuses because I know everyone is busy, so I'd prefer to have testable, full featured PRs rather than smaller, but harder to test without context, PRs that potentially block further work.
Any API changes or non-block specific components will be raised on master separately, and then rebased here once merged. I am not creating additional branches for review off of this branch. I imagine I'll change this to a proper PR and request review when it satisfies #2392 I mainly created a draft to track progress (this PR is marked draft/in-progress), and so something is visible whilst work on components is done at the same time in other PRs.
I don't think we need to discuss this here and now, but we should really have 3 flags IMO. development, feature plugin, and core fwiw. If current flag usage is documented somewhere let me know so I can read up on it's proper purpose. Obviously creating this draft has caused some confusion, so I can close it and raise again when I'm ready if you like? I didn't intend to kick off a large workflow debate. |
|
My apologies, I'm pretty sure I misunderstood the purpose of this pull then :) I thought you were intending on using this for a long running feature branch (and I also missed the important indicator that this was a draft pull) 🤦 |
mikejolley
force-pushed
the
add/single-product-block
branch
from
ac263dd to
56a7277
Compare
Add preview state for single product blockAdd preview state for single product block https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/f88c31cb6a03c0b488485738dca276b45b98bda0/assets/js/blocks/single-product/edit.js#L85-L96🚀 This comment was generated by the automations bot based on a
|
|
The latest commit I pushed (202e5a2) fixes an issue where the context was not being seen by atomic components within the atomic blocks for the single product block. Essentially the problem was that the single product block was using an instance of The fix, is to push the Going forward, we should probably take some time to figure out how we want to approach this because there are implications either way. Build a bundle for shared context.Anytime we share context across bundles that implement the context wrapping atomic blocks, we should put that context in the new shared context build I did in the commit. Pros:
Cons:
Other options?I thought, maybe we could have atomic blocks built to their own bundle. If we went this way, it still won't work because we'd run into the same problem we have now with the context defined in the atomic blocks bundle being different than that used by the context provider defined in the various parent block bundles. The only way around this would be to do some fancy build alias swapping so editor builds pull the context from the atomic blocks external and frontend builds pull from the context from the main file. What should we do?Since any approach I can think of results in differing builds between frontend and backend, I think we should just go with the approach I implemented here for now but only implement usage of the external for editor builds. On the frontend we can alias Eventually, we're going to have to revisit this when/if we implement InnerBlocks in cart and checkout. That will be tricky because all the hooks used in the cart and checkout contexts create some circular dependencies when the contexts are built to a separate build. |
There was a problem hiding this comment.
Wow, this looks really great @mikejolley, good job! I'm super excited about this block. 🎉
I didn't take a look at all the code yet, but will leave some early feedback about some things I noticed and that I couldn't find in the follow-up issues. If you were already accounting for them, feel free to ignore this comment. 🙂
- Product Image: The Sale badge doesn't seem to work for me:
- Product Title: the Link to Product Page attribute doesn't have any effect in the frontend.
- I'm not allowed to add inner blocks specific to the product (in the screenshot, I was trying to add back the Product Summary block).
- The position of inner blocks is not persisted in the frontend:
| Editor | Frontend |
|---|---|
 |
 |
This is due to using columns blocks which have their own Inner Block configs. We'll need to look at this as part of #2526, either fixing columns or adding a custom column block. |
There is a bug here, but also it could be when a product is not on sale. This seems to be something we'll need to work out of the design side since we're working with live data cc @LevinMedia |
| @@ -0,0 +1,150 @@ | |||
| /** | |||
| * External dependencies | |||
There was a problem hiding this comment.
Moved contents of Atomic Components to a block.js per atomic block. This is so we can reuse the same block for edit.js and frontend.js, and also allows frontend.js to see the defined blockAttributes so it can map dataset to block attributes correctly.
| /** | ||
| * Internal dependencies | ||
| */ | ||
| export const blockAttributes = { |
There was a problem hiding this comment.
Defining these separately so the frontend.js can validate attributes against them.
|
|
||
| const Save = ( { attributes } ) => { | ||
| return ( | ||
| <div className={ classnames( 'is-loading', attributes.className ) } /> |
There was a problem hiding this comment.
This is new; saves some markup for the InnerBlock so we can render from this on the frontend instead of using layoutconfig.
| * | ||
| * @param {Array} template Inner Blocks Template. | ||
| */ | ||
| export const createBlocksFromTemplate = ( template ) => { |
There was a problem hiding this comment.
This utility will create blocks recursively from a defined default template. Used currently to reset the layout back to default.
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import ProductButton from '../blocks/product/button/frontend'; |
There was a problem hiding this comment.
Rather than using atomic/components, this is mapping to the frontend.js file of each defined Atomic Block. The frontend.js file is responsible for converting dataset attributes to block attributes, then rendering the block's component.
| * | ||
| * @param {Object[]} innerBlocks Inner block components. | ||
| */ | ||
| export const getLayoutConfig = ( innerBlocks ) => { |
There was a problem hiding this comment.
Moved from base-utils. This might be removable in a follow up when layout configs are deprecated/removed.
| * @param {Object[]} layoutConfig Object with component data. | ||
| * @param {number} componentId Parent component ID needed for key generation. | ||
| */ | ||
| export const renderProductLayout = ( |
There was a problem hiding this comment.
Moved from product-list-item/utils.js
| return ( | ||
| <LayoutComponent | ||
| key={ keyParts.join( '_' ) } | ||
| attributes={ attributes } |
There was a problem hiding this comment.
This was changed to maintain compatibility with the atomic blocks - before we just passed all props.
| @@ -0,0 +1,148 @@ | |||
| /** | |||
| * External dependencies | |||
There was a problem hiding this comment.
These utils were moved form assets/js/utils since they are shared and used throughout.
| * @param {string} props.keyPrefix Prefix for keys. | ||
| * @param {Object} props.blockMap Child blocks will be mapped to components. | ||
| */ | ||
| const renderChildren = ( { children, blockMap, keyPrefix = '' } ) => { |
There was a problem hiding this comment.
This is where the new magic happens; this converts inner blocks into usable components.
This fixes an issue where blocks that use contexts are using different instances
* renderChildren method with enhancement to renderFrontend * Update renderFrontend usage * Remove core components from block map * Register atomic blocks with save method * Atomic components to use context * Break out single block save method * Update singlge product * Atomic PHP side registration * Fix element attributes * Undo atomic block reg changes * Implement reset layout * Tweak loading states
mikejolley
force-pushed
the
add/single-product-block
branch
from
09cd9d2 to
a2121de
Compare
Attepting diff cleanup
This reverts commit 88b1893.
@mikejolley I'll be looking into innerblock settings this week, and I'll be sure to give that one some thought. |
Creates the basic structure for the Single Product Block (behind the experimental flag).
Closes #2392
Closes #2535
Introduces a new hook nameduseSyncedLayoutConfigwhich syncs inner block changes to block attributes as a LayoutConfig array. This is only being used by Single Products Block currently. The All Products block does not update instantly and so does not need this (yet)useProductLayoutContextanduseInnerBlockConfigurationContextsince both are used at the same time in all contexts. This allows the parent block to be passed, as well as a prefix for the CSS classnames.ProductDataContextto share real product data with atomic blocks.ProductDataContext.Known follow ups:
Screenshots
Edit interface:
Loading state:
Inner Block state:
How to test