-
Notifications
You must be signed in to change notification settings - Fork 46
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
storybook-django: Storybook prototype of the pattern library #103
Comments
Storybook prototype reviewWe’ve reviewed this together with @bcdickinson and @William-Blackie today – here are things to be addressed / to think of further,
And clear improvements that this brings:
The takeaway from our conversation is that this comes with clear improvements on multiple fronts (UI, workflow, constraints on file structure), and doesn’t make other areas of the pattern library worse, beyond the initial setup. There’s more for us to research (cc @bcdickinson) on whether this would make it easier for us to address the pattern library’s limitation to basic data structures (as opposed to Python-specific types / Django objects), and whether it could help address the limitation of having to hard-code a lot of mock data (integration with factories). @bcdickinson I think this would be a good place to share your findings once you’ve had the time to look into this more. |
I’ve experimented with this a bit more since writing the last comment and found something that seems to work quite well – automatically generating stories based on the project’s YAML files. There is a barebones example of this at https://github.com/torchbox/storybook-django/blob/master/demo/storybook/legacy.stories.js, and here is a more advanced example: View codeimport { storiesOf } from '@storybook/react';
import React from 'react';
import { text, number } from '@storybook/addon-knobs';
import { TemplatePattern } from 'storybook-django';
import '../../project_styleguide/templates/patterns/base.html';
import '../../project_styleguide/templates/patterns/base_page.html';
const req = require.context(
'../../project_styleguide/templates/patterns',
true,
/\.yaml$/,
);
const knobify = (obj) => {
return Object.keys(obj).reduce((cont, key) => {
const val = obj[key];
if (typeof val === 'string') {
cont[key] = text(key, val);
} else if (typeof val === 'number') {
cont[key] = number(key, val);
} else {
cont[key] = val;
}
return cont;
}, {});
};
req.keys().forEach((path) => {
/* eslint-disable global-require, import/no-dynamic-require */
const yaml = req(path);
const cleanPath = path.replace('./', '').replace('.yaml', '');
const pathElts = cleanPath.split('/');
const filename = pathElts.pop();
const source = require(`../../project_styleguide/templates/patterns/${cleanPath}.html`);
const rawYaml = require(`!!raw-loader!../../project_styleguide/templates/patterns/${cleanPath}.yaml`);
const context = yaml?.context ?? {};
const usage = Object.keys(context)
.map((key) => {
const val = context[key];
const wrappedVal = typeof val === 'string' ? `"${val}"` : val;
return `${key}=${wrappedVal}`;
})
.join(' ');
const folders = pathElts.join('/') || 'Templates';
storiesOf(folders, module).add(
filename,
() => {
return (
<TemplatePattern
template={`patterns/${cleanPath}.html`}
context={knobify(context)}
tags={yaml.tags}
/>
);
},
{
notes: {
markdown: `
### Usage (experimental)
\`\`\`html\n{% include "patterns/${cleanPath}.html" with ${usage} %}\n\`\`\`
### Source
\`\`\`html\n${source.default}\n\`\`\`
### YAML
\`\`\`yaml\n${rawYaml.default}\n\`\`\``,
},
},
);
}); This has a number of advantages:
Doing this, I also realised a few shortcomings of the prototype:
|
I've added this to BCUK (although have not made an MR based on this - I can do if that would be useful) and have had a good look around. My findings: Good 👍
Observations 👀
Thoughts 🤔
|
Thank you @chris-lawton 🤘 This is very useful feedback! I’ve removed links straight to the project’s code from your comment. I don’t think this is particularly problematic but since this is all public I’d rather be extra cautious. Might be worth raising with the sysadmin whether this kind of caution is warranted or not. For JS – I suspect this is just because JS for each component isn’t loaded in the Storybook version. This is something I didn’t implement with the sample code I sent you. The vanilla pattern library renders each component as if they were inside the project’s …which (for most builds) includes the project’s stylesheet, JS, SVG icons. For the Storybook version, by default patterns are rendered directly without any base template, so we need to load the CSS & JS manually, as well as any other component dependencies. Inside the Storybook import { configure, addDecorator, addParameters } from '@storybook/react';
import { withA11y } from '@storybook/addon-a11y';
import iconSprite from '../../project_styleguide/templates/patterns/atoms/sprites/sprites.html';
configure(() => {
const hasIcons = document.querySelector('[data-storybook-svg-icons]');
if (!hasIcons) {
const icons = document.createElement('div');
icons.setAttribute('data-storybook-svg-icons', true);
icons.innerHTML = iconSprite;
document.body.appendChild(icons);
}
// eslint-disable-next-line global-require
require('../sass/main.scss');
}, module); When I get back to this I’ll check what would be the best way to load JS, whether it’s as simple as |
Here is a short update on this prototype. TL;DR;It seems to be working very well. I personally feel more productive with it than with DPL’s YAML files. Highlights
Compared with django-pattern-library re the above comments,
import { Pattern } from 'storybook-django/src/react';
export default {};
export const Base = () => <Pattern filename={__filename} />; This is the equivalent of not having a Known issuesThere is only one I’m aware of compared to django-pattern-library – the current API to render a template only supports passing context and tags for the currently-rendered template, not any of its dependencies. For vanilla DPL this is only an issue in a few cases (#138, #8). Next stepsI’m interested in trying out a version of DPL with the Storybook UI but without the runtime dependencies. This is dependent on the release of Storybook v7, as I want to make sure to not rely on any internal or deprecated APIs that might get removed ( |
The prototype has been further trialed by @stevedya – we ran into #138, #193, and found an unrelated django-pattern-library bug: #199. We tried this with Storybook v6.5, which is much lighter than previous releases. The next release, v7.0, promises to be even lighter, which might alleviate some of the concerns with using npm dependencies for this in addition to the Python django-pattern-library. This was done on the Wagtail project, which uses storybook-django. Here is a PR showing the work involved: wagtail/wagtail#8665. |
Tracking issue for conversations relating to storybook-django, a prototype Storybook integration of the pattern library.
Related issues
All of the issues tagged
storybook
are related to this integration – either because the Storybook UI addresses them, or because using the pattern library through such an integration would drastically change the implementation.Addressed by Storybook
Here are open issues that would no longer be relevant when using the pattern library with Storybook:
And issues for which the implementation would be drastically simpler:
The text was updated successfully, but these errors were encountered: