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

WIP: Enable custom html in iframe <head> #77

Merged
merged 3 commits into from
Apr 11, 2016

Conversation

karl
Copy link
Contributor

@karl karl commented Apr 7, 2016

This is a work in progress to enable adding custom html to the iframe that the components are rendered in.

This is useful for loading fonts from Typekit (which require specific script tags).

The idea was brought up in this issue: #61

@karl
Copy link
Contributor Author

karl commented Apr 7, 2016

There are currently no tests or documentation for this. I just wanted a sanity check that I'm on the right path before spending too long on this.

Any and all feedback on this is much appreciated!

@jeef3
Copy link
Contributor

jeef3 commented Apr 7, 2016

Cool, looks like a reasonable way of doing it. Just allow the user to supply their own head.html and that gets injected.

There's probably a nicer way of doing it, but I think this would go a long way to helping 👍

@@ -1,9 +1,10 @@
export default function () {
export default function (extraHtml) {
Copy link
Member

Choose a reason for hiding this comment

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

May be rename as extraHead

@arunoda
Copy link
Member

arunoda commented Apr 7, 2016

Yes.This is the way it should be. We don't have any tests for the server.
It's great, if you can start it with this PR. (Just for this case).

@karl
Copy link
Contributor Author

karl commented Apr 8, 2016

@arunoda I've added tests for the new functionality. I'm inexperienced with testing JS on the server, and wasn't sure how best to test the function. I included mock-fs to mock the file system while testing, let me know if that is not the preferred approach!

I'll look at adding some documentation next, although I'm not sure of the best place to put it. Any where you would prefer to add it?

@@ -0,0 +1,12 @@
import path from 'path';
Copy link
Member

Choose a reason for hiding this comment

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

Shall we move this into a file called src/server/utils.js and export the following function as getHeadHtml.

@arunoda
Copy link
Member

arunoda commented Apr 9, 2016

@karl Everything looks great.
Once you move the content as I suggested, we are good to go.

@arunoda arunoda merged commit d6794d1 into storybookjs:master Apr 11, 2016
@arunoda
Copy link
Member

arunoda commented Apr 11, 2016

I've done the suggested I made and merged.

@karl
Copy link
Contributor Author

karl commented Apr 11, 2016

Awesome, thanks @arunoda!

@arunoda
Copy link
Member

arunoda commented Apr 12, 2016

Thanks you too.

@jeef3 jeef3 mentioned this pull request May 4, 2016
ndelangen pushed a commit that referenced this pull request Apr 5, 2017
ndelangen pushed a commit that referenced this pull request Apr 5, 2017
#77)

* Modify number() to support a range slider as an input type for a knob.

* Style changes and lint fixes.

* Remove unwanted vars
ndelangen pushed a commit that referenced this pull request Apr 5, 2017
@shilman shilman added the misc label May 27, 2017
ndelangen pushed a commit that referenced this pull request Feb 23, 2024
…-on-configure-page

Replace chevron icon on Configure page to avoid @storybook/components usage
Copy link

nx-cloud bot commented May 31, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d6794d1. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants