-
-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
Code injector addon to dynamically inject code in the iFrame #4152
Conversation
Makes sense to me |
Please add some docs on usage |
} | ||
|
||
static getDerivedStateFromProps(props) { | ||
return { resources: props.resources }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's called on every setState
in React 16.4+. So state.resources
will always be the same as props.resources
Thanks @Hypnosphi, i'm probably going to change a lot in here. but thanks for the initial feedback. |
Hey Nevilla, I know others are requesting a similar addon. Think you'd have time to work on this more or share the work with someone else? |
Hey @ndelangen , let me try to finish or atleast get it to a point where someone else could take over 👍. Let me know if you have any deadlines as far as releases go.. |
No deadlines apply to new addons. |
Codecov Report
@@ Coverage Diff @@
## next #4152 +/- ##
==========================================
- Coverage 35.23% 34.88% -0.36%
==========================================
Files 564 567 +3
Lines 6924 6994 +70
Branches 929 937 +8
==========================================
Hits 2440 2440
- Misses 3992 4054 +62
- Partials 492 500 +8
Continue to review full report at Codecov.
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be listed in
- https://github.com/storybooks/storybook#addons
- https://github.com/storybooks/storybook/blob/master/ADDONS_SUPPORT.md
(Also readme is missing)
); | ||
|
||
storiesOf('Addons|Resources', module) | ||
.add('Primary Large Button', myButton('Primary Large Button', 'btn btn-lg btn-primary')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you use this strange components syntax? I didn't see it in use in the other stories... I think we need to be consistent with the examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to make it consistent with some of the other examples i see..
addons/resources/src/index.js
Outdated
name: 'withResources', | ||
parameterName: 'resources', | ||
skipIfNoParametersOrOptions: true, | ||
allowDeprecatedUsage: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not. changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not (& changed to false).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not (& changed to false).
import PropTypes from 'prop-types'; | ||
import { Button } from '@storybook/components'; | ||
import styled from '@emotion/styled'; | ||
import AceEditor from 'react-ace'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for the future: I think we need to stick to a single editor in all these code highlighting components.
- Storysource has now a
react-syntax-highlighter
which is not an editor thought, - Here it's an
AceEditor
- IMO @libetl is adding another one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not yet decided whether to add codemirror or monaco for the moment.
The change will probably happen in the tech/overhaul-ui branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, in future we can change it to whatever we decide on.
/> | ||
</div> | ||
<div style={{ float: 'right', 'padding-top': '17px' }}> | ||
<Button onClick={this.apply.bind(this)}>APPLY RESOURCES</Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to have a "Disable" button ? Looks like it's needed to delete everything from the editor to disable the resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added one.
loadResources() { | ||
const { resources = `` } = this.state; | ||
|
||
const addElements = (domElements, i) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this function could be extracted, it doesn't use any inner state from the loadResources
func
ADDONS_SUPPORT.md
Outdated
@@ -15,6 +15,7 @@ | |||
|[links](addons/links) |+|+|+|+|+|+|+| |+|+|+| | |||
|[notes](addons/notes) |+|+*|+|+|+|+|+| |+|+|+| | |||
|[options](addons/options) |+|+|+|+|+|+|+| |+|+|+| | |||
|[resources](addons/resources) |+|+|+|+|+|+|+|+|+|+|+| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can assume that RN is not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
@nm123github, you need to update the snapshots |
IDK how but looks like |
Just had a brief chat with @shilman . This will go in 4.1 (so lets not merge for now). |
Yeah that's what I feel is best. also. Let's release 4.0.0 first. |
@nm123github is anything missing here ? |
Adding css is probably a much more common use case which should be covered in the addon-cssresources. Also, looks like there's a PR being worked on to integrate monaco-editor so in anycase probably best to wait for that. Maybe will resurrect this sometime in future if there's a need. |
@nm123github what is the PR that integrates monaco-editor? |
@playma256 has been working on a tiny lightweight story editor, but focus has shifted a bit. The storyState was changed for Args & my time was spend on the composition feature. I'm sure we'll work on a true editor within storybook Q2. The status is "on hold" |
Issue: At this point i don't believe there is a way to add remove css resources dynamically in a running storybook instance. I think there might be use-cases where folks want to check if a component works fine in different versions of a css/js library.
What I did
GIF demo
There is still work to be done on this. Just wanted to get some preliminary comments/feedback. Also, it probably wont be trivial to swap in/out javascript versions so mostly this will probably work with just css (at-least to begin with).
Updated
Updated PR with new code and details below (in comments). Please ignore ^ initial details and demo.