Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addon-docs: Support for iframeWidth parameter on stories #9149

Closed
wants to merge 2 commits into from
Closed

Addon-docs: Support for iframeWidth parameter on stories #9149

wants to merge 2 commits into from

Conversation

alonsosfera
Copy link

Issue: #8816

What I did

Enabled Story components to receive parameter iframeWidth to configure the width of the iFrame shown in the Docs page

How to test

Add parameter iframeWidth to /storybook/examples/angularcli/src/stories/app.component.stories.ts like shown below:

image

Run Angular-cli example, go to App Component -> Component with separate template -> Docs page and watch width of the iFrame changed as shown below.

image

  • Is this testable with Jest or Chromatic screenshots? ... no
  • Does this need a new example in the kitchen sink apps? ... no
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@vercel vercel bot temporarily deployed to staging December 13, 2019 22:19 Inactive
@vercel
Copy link

vercel bot commented Dec 13, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/54vd47zeb
🌍 Preview: https://monorepo-git-fork-alonsosfera-feature-addons-docs-iframewidth.storybook.now.sh

@alonsosfera
Copy link
Author

@tmeasday @ndelangen Any idea why that test fails? I did some research and danger-js seems to that have an ongoing issue related to that. (danger/danger-js#918)

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Lots of small issues with the PR. Can you take a look?

@@ -7,5 +7,7 @@ export interface AnchorProps {
}

export const Anchor: FC<AnchorProps> = ({ storyId, children }) => (
<div id={anchorBlockIdFromId(storyId)}>{children}</div>
<div id={anchorBlockIdFromId(storyId)} style={{ textAlign: 'center' }}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why text-align center?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this so no matter what size the iFrame has, the component will always be centered on the Docs Page...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without it:
image

With it:
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that visual outcome looks ok to me (I'm no designer), but this shouldn't be done with inline styles IMHO, I'm wondering if centering should be done with flexbox rather then text-align.

@shilman @domyen does the centering of the iframe seem like the right thing to do to you?

@@ -71,14 +73,15 @@ export const getStoryProps = (
`Story '${storyName}' is set to render inline, but no 'prepareForInline' function is implemented in your docs configuration!`
);
}

return {
const newVar = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is newVar?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After adding the width property to the StoryProps interface, TypeScript kept giving me the following error:
image

I kept getting the error whenever I would return the object like this:
image

After inserting the object into a constant and then returning it, the error cleared out.
image

@@ -54,6 +54,7 @@ const PreviewContainer = styled.div<PreviewProps>(
position: 'relative',
overflow: 'hidden',
margin: '25px 0 40px',
display: 'inline-block',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this needed?

Copy link
Author

@alonsosfera alonsosfera Dec 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is necessary in order for the preview container to adjust to the iFrame's width.

Without it, it would look like this:
image

Instead of this:
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using flexbox for this is a little more robust I think.

id,
title,
height = '500px',
width = '950px',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this completely optional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's actually just a default height and width. In case the IFrameStory component does not receive those parameters, it would just set it to 950x500 px...

Could you explain a little bit more about how this could be optional?

@alonsosfera
Copy link
Author

@shilman Do you mind taking a look at it?

@shilman
Copy link
Member

shilman commented Dec 18, 2019

@alonsosfera busy trying to get 5.3 out right now. will take a look as soon as i've worked through my backlog. thanks for your patience!

@stale stale bot added inactive and removed inactive labels Jan 10, 2020
@ndelangen ndelangen changed the base branch from next to next-6.0.0 January 11, 2020 01:38
@stale stale bot added the inactive label Feb 1, 2020
@shilman shilman self-assigned this Feb 1, 2020
@stale stale bot removed the inactive label Feb 1, 2020
@shilman
Copy link
Member

shilman commented Feb 1, 2020

Picking this up this week. Sorry for the delay and thanks for your patience @alonsosfera

@storybookjs storybookjs deleted a comment from stale bot Feb 3, 2020
@storybookjs storybookjs deleted a comment from stale bot Feb 3, 2020
@ndelangen ndelangen self-assigned this Jun 4, 2020
@ndelangen
Copy link
Member

I can't work out the merge conflicts, what can we do about this PR @shilman ?

@shilman
Copy link
Member

shilman commented Jun 5, 2020

I'll take this one @ndelangen

@shilman shilman removed this from the 6.0 milestone Jul 30, 2020
@shilman shilman added this to the 6.1 docs milestone Jul 30, 2020
@m1lk1way
Copy link

hi, guys, it would be nice to have this. Are you going to finally merge it?

@shilman
Copy link
Member

shilman commented Aug 27, 2020

@m1lk1way yup, will merge in the next few days and release as 6.1-alpha

@shilman shilman modified the milestones: 6.1 docs, 6.2 docs Nov 24, 2020
@shilman
Copy link
Member

shilman commented Dec 1, 2020

Looks like the fork for this has been deleted and the PR is not in a mergeable state, so I'm going to close this for now.

@shilman shilman closed this Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants