Addon-docs: Fix fixed-position inline stories #11350
Conversation
lib/components/src/blocks/Story.tsx
Outdated
| @@ -34,12 +35,16 @@ type IFrameStoryProps = CommonProps; | |||
|
|
|||
| type StoryProps = InlineStoryProps | IFrameStoryProps; | |||
|
|
|||
| const InlineStory: FunctionComponent<InlineStoryProps> = ({ storyFn, height, id }) => ( | |||
| const InlineStory: FunctionComponent<InlineStoryProps> = ({ storyFn, height, id = uuidv4() }) => ( | |||
There was a problem hiding this comment.
Can id be undefined ? Seems weird ?
Appart from this, I don't think you need uuid for this. This adds another dependency for something simple. Interesting reading about it
Math.round(Math.random() * 1e5) will be enough, unless someone plans to have that many story, we can take the risk of having the same id twice :)
There was a problem hiding this comment.
Yes, it can be undefined, using InlineStory directly, that's why!
Really interesting reading and I'm not against removing this extra dependency (even if it's already used in other packages)
There was a problem hiding this comment.
Thanks for the explanation ;)
I'm sure we can apply the same logic for other use of uuid.
lib/components/src/blocks/Story.tsx
Outdated
| <div className={`story--${id}`}> | ||
| <Fragment> | ||
| {storyFn ? createElement(storyFn) : <EmptyBlock>{MISSING_STORY(id)}</EmptyBlock>} | ||
| </Fragment> | ||
| </div> |
There was a problem hiding this comment.
There's no way to do this without the extra <div>?
There was a problem hiding this comment.
I don't see how actually but this Fragment is not necessary anymore if we keep this extra div 🤔
There was a problem hiding this comment.
@ndelangen Can it fit as it?
There was a problem hiding this comment.
@ndelangen any concerns?
|
I do fear the extra div is a change that will bite us later.. |
|
What about an absolute div before the story with the given constraints ? I assume setting the parent of stories relative is no-impact or at least something we can explain. |
|
Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks! |
|
@ndelangen @frassinier any opinion on my potential solution ? |
|
@tooppaaa I'm personally against using absolute position when it's not necessary but thank you for the proposal. I can rework it for resetting the z-index context for each story instead... |
frassinier
force-pushed
the
fix/addon-docs/fixed-position
branch
from
1f5e1b3 to
afd88b4
Compare
|
Wait Clément still in progress 🤞 |
|
@tooppaaa, @ndelangen, and @shilman 👋 I hope it will fit this time, I can't do it simpler! |
shilman
changed the title
Issue #8011
What I did
If you provide height to the story component it might want to deal with some fixed-positioned elements so we reset the position context.
How to test