Create shared contexts handle for inner blocks #2568
Conversation
mikejolley
added
status: needs review
type: refactor
mikejolley
requested review from
haszari and
nerrad
and removed request for
a team
|
Size Change: +618 B (0%) Total Size: 1.99 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Looks like we maybe have a problem - I'm seeing this error on front end:
By the way, this is a tricky PR to review. I'd really appreciate a bit more rationale/detail about the intention in this PR description. I realise that this may be covered in the linked comment but it's easy to get rabbit holed. It's ideal if we can split of a chunk of a PR and an average team member can review it in isolation, and understand what the goal is :)
This takes extra time and effort for sure – abstraction is hard, docs are hard – but I think it's worth the effort :)
#2399 is a mammoth PR so I'm breaking out some fixes and changes to review separately.
This is a pretty mammoth change on its own! 😅
|
... @haszari that error is identical to the one you posted in #2441 so it seems to me it's unrelated to the changes in this PR.
The linked comment seems clear enough. I'm not sure what more explanation I can offer on top of the comment from @nerrad. In case you missed it, this is the key bit of information about the change to be aware of:
Sounds like you were just derailed by the error message?
¯_(ツ)_/¯ It's mostly moving code around. |
Yep totally – apologies, I guess this is a risk when we have known issues like this. Thanks for highlighting the "context" (pun intended!) - still not fully clear on the details. For example, I'm not familiar with |
There was a problem hiding this comment.
Gave All Products & friends a good test on front end and in editor.
- Existing blocks loaded fine in editor.
- Existing blocks loaded fine and worked correctly on front end (filtering).
- Changed
All Productsoptions - rows, align etc - persisted and worked on front end. - Changed
All Productsinner blocks, worked correctly on front end. - Quick test of front end in Safari & Firefox.
Didn't see any issues :)
Regarding the context / provider issue - my understanding (from reading the comments) is that we should have a single context instance across all blocks (within a page load) (aka singleton pattern?), and the issue here is that because of the way we were building/bundling, we ended up with two contexts for different kinds of block.
Is there a pattern/convention we could use to prevent/detect issues like this happening in future? How do you spot this kind of problem in a review, or how would it show up as bugs in e2e or manual testing?
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.
Though it looks like we're still working out the best/right approach here, and there are quite a few considerations - tricky :)
I think we'll need to document these more after Inner Blocks work is concluded. There is a readme file for each context however. This one provides a classname for contained inner block components (so they all get the same prefix) and a parent block name.
Yup :)
It's difficult - I spotted this when my context values were not picked up by the nested components. This occurs because each frontend block has it's own script file. |
Aljullu
added
the
skip-changelog
#2399 is a mammoth PR so I'm breaking out some fixes and changes to review separately.
Background behind these changes can be seen in this comment from @nerrad #2399 (comment)
Changes:
How to test the changes in this Pull Request:
This isn't in use by multiple blocks yet, so to test please just Smoke Test the All Products block and ensure there are no regressions in the layout (front and backend).