fix(stories): With Layer story props not updating#21716
fix(stories): With Layer story props not updating#21716sangeethababu9223 wants to merge 8 commits into
Conversation
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21716 +/- ##
==========================================
+ Coverage 87.66% 87.70% +0.03%
==========================================
Files 536 536
Lines 43634 43634
Branches 6721 6723 +2
==========================================
+ Hits 38253 38269 +16
+ Misses 5230 5214 -16
Partials 151 151
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…://github.com/sangeethababu9223/carbon into fix/with-layer-inconsistant-behaviour-stories
devadula-nandan
left a comment
There was a problem hiding this comment.
Hey @sangeethababu9223,
IMO, this feels a bit over-engineered to me.
Storybook provides callable story() methods that can be used to clone components, which seems like the best fit for this scenario. I’m not a huge fan of attaching mutation observers to the story root for something that’s purely representational.
I have a draft PR using the decorator approach:
#21726
Open to discussion, what do you think ?
is there anything missing from this decorator approach ?
Hey @devadula-nandan , |
|
Closing this PR as a much cleaner implementation is done in #21726 |
Closes #20095
With Layerstories do not respond to controls beyond the first layer. An initial fix was introduced in #21267. However, that update did not account for nested elements or components with children.Two solutions are introduced in the PR:
Enhanced Clone-and-Sync Approach
The existing implementation has been updated to sync attributes for child elements in addition to the root component. This solves the problem to some extent, but the sb-template-layers Storybook template component's clone-and-sync approach has a fundamental limitation:
The MutationObserver cannot differentiate between:
This causes issues with interactive components like multi-select, where clicking the top layer component would inadvertently trigger state changes in the cloned layers below, resulting in focus jumping and unexpected behavior.
Workaround:
This array prevents syncing of interaction-based attributes, but it is not a proper solution as it requires manual maintenance and is brittle.
Render Function Approach
Introduced a new renderContent property that accepts a render function, enabling a declarative approach:
Usage:
Benefits:
This implementation eliminates the challenges associated with attribute synchronization and is much cleaner. While a slot-based approach is generally preferable in Web Components, in this case, the render function approach is more beneficial because the chances of unwanted side effects are significantly lower compared to the existing implementation.
Trade-off:
The only downside is that all Layer stories across components would need to be updated to use this approach. To address this, the original code has been preserved, and renderContent has been added as a new option, allowing both approaches to coexist.
Changelog
New
renderContentbased implementation option in the layer template.Changed
The following changes have been made to
packages/web-components/.storybook/templates/with-layer.ts:skipAttributes.Testing / Reviewing
Review the With Layer story for any component that has controls enabled. Changes made through the controls should be reflected across all layers, not just the top layer.
The following components previously had issues in their With Layer stories, and the behavior can be verified there:
Also, verify the Multi select With Layer stories to ensure the new implementation works as expected.
PR Checklist
As the author of this PR, before marking ready for review, confirm you:
Updated documentation and storybook examplesWrote passing tests that cover this changeAddressed any impact on accessibility (a11y)More details can be found in the pull request guide