-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Plugin API first draft #505
Conversation
Codecov Report
|
@@ -71,21 +71,24 @@ export default class Playground extends Component { | |||
preview={<Preview code={code} evalInContext={evalInContext} />} | |||
tabButtons={ | |||
<Slot | |||
name="exampleTabButtons" | |||
name="exampleTabButton" |
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 a more specific name like "playgroundTabButton" would be better here
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.
👍
// { id: 'pizza', render: ({ foo }) => <div>{foo}</div> } | ||
const rendered = fills.map(({ id, render }, index) => { | ||
// Render only specified fill | ||
if (onlyActive && id !== active) { |
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'm afraid I don't fully understand this bit. Can there be a case when active
is set, but onlyActive
isn't? What would be the intention of it?
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.
Yup, tab buttons for example: we need to show all of them but highlight only one.
@@ -9,30 +9,26 @@ export default function Slot({ name, active, onlyActive, className, props = {} } | |||
throw new Error(`Slot "${name}" not found, available slots: ${Object.keys(slots).join(', ')}`); |
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.
The logic of this warning is probably incorrect: it warns if an existing slot can't find any fills (which should be totally ok: you could have an extension point but nothing occupying it at the moment), but instead it should warn if a fill requests a slot that doesn't exist.
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.
All slots are empty objects by default, so the check will work (it’s already working in 5.4 ;–)
This looks like a great first step. Some parts are a bit unclear to me, like how would a third-party plugin be registered (webpack loaders will have to be reconfigured for this, probably?) and configured, but with a bit of documentation it should be fine. I think it would be great if styles or classnames were somehow exposed to the plugins, so that they could use them to look like a natural part of the UI. A third-party button plugin will not have easy access to TabButton component and won't be able to easily follow any future changes in RSG UI (or any customizations that an end-user might have). Passing refs for relevant HTML nodes would be useful, too. |
Like this, via style guide config: module.exports = {
plugins: {
// These are Node module names
'my-awesome-plugin': true,
'my-another-plugin': {
// Some options here
}
}
};
Plugins are run in the same webpack context as the Styleguidist UI so you could import
Not sure about that yet ;-) |
Are plugins going to be loaded in alphabetical order? Should |
Alphabetical doesn’t make any sense ;-) It should work similar to PostCSS. Or we can make an array, similar to Babel: module.exports = {
plugins: [
// These are Node module names
'my-awesome-plugin',
['my-another-plugin', {
// Some options here
}]
]
}; |
Yes, I think the babel way is good. |
Cool, I’m fine with it too — both are familiar for many users. |
Well done, @sapegin. My only concerns are about the props the plugins' components receive (I'm thinking about Snapguidist).
|
All fills in a slot receive the same props except
There’s no such action ;-) Tabs are using the same
It should be possible and most likely will be covered by that, at least partially. But I need more feedback and ideas here ;-| @MicheleBertoli |
I have played a bit with this branch and it looks promising. It is easy to start developing a test plugin: just checkout the branch and work in one of the examples. The plugin loader happily accepts local paths so you don't have to touch npm while you develop: // in styleguide.config.js
plugins: {
//...
'./plugins/test': {
options: { /* plugin configuration options */ }
}
} The configuration options from above can be accessed in your plugin like this: // in ./plugins/test/index.js
//...
const testPlugin = (globalConfig, pluginConfig) => {
// plugin code
}
export default testPlugin; I haven't tried doing this yet but it looks like you can export multiple components (or "fills") from one plugin module and they will share the module code and state, @MicheleBertoli. I think that the next steps for this proposal should be:
|
Feel free to do it if you have time! ;-)
Yup, right now they receive some random data ;-|
If it makes implementation simpler why not?
I think we should try to implement some useful plugins and then we’ll see what work and what doesn’t.
I think JSS dynamic properties is a way to go here. We’ll need to pass more data to some component and make plugins be able to pass styles.
Something like that will be necessary. I’d rather pass
It supposed to work already, I’ve probably missed something ;-) |
loadPlugins.js expects |
But this is a check for a plugin itself, not for a fill. I don’t see any checks for fills. |
...right. Huh. I guess I'm doing something wrong somewhere else. |
It may be me I just don’t see where yet ;-) |
What do you think about swapping the order of arguments in plugin constructor? Now it is |
Maybe, I don’t have a strong preference ;-) |
@sapegin, I've added two commits to change the order of arguments and change the object into array. |
scripts/schemas/config.js
Outdated
type: 'object', | ||
default: {}, | ||
type: 'array', | ||
default: [], | ||
process: val => | ||
CORE_PLUGINS.reduce((allPlugins, plugin) => { |
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 now just concat
should be enough ;-)
I’ve merged the master branch here, going to add a few more things and release with 6.1.0. It will be very alpha but you’ll be able to implement some very basic plugins. |
I’ve also fixed the CI accidentally ;-) |
Doesn’t work well with React 16 — hides the actual error message.
…e* slots to playground*
@isratc That’s an interesting way of managing open source ;-) I’m planning this for 6.1.0 which depends on several PRs so I can’t say anything more that ”soonish”. Could you share your use case? Is there any missing features? It’s very very limited for now. In any case you can try to write your plugin using this branch — your feedback will be very valuable. |
@isratc I’m not sure what you’re trying to do and how is it related to Styleguidist plugins API. Please open a new issue and make a demo project that we can debug based on this repo: https://github.com/styleguidist/example |
You might consider using the plugin system from MrBuilder |
@sapegin any plans to merge it? |
I'll need some help to make it ready, even as an MVP. |
😴 This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week without any further activity. Thank you for your contributions. |
Oh :( sad to see this MR is closed |
plugins
config optionI have no idea how to do:
Wrapper
component).