docs: add script to fetch design system pages for storybook #558
Conversation
This comment has been minimized.
This comment has been minimized.
|
Looks really good 👍 Maybe we should update the links to the principles in https://github.com/dhis2/ui/blob/master/docs/constants.md to point to the storybook versions? |
|
Yeah that sounds good 🙂 |
|
Good idea @KaiVandivier. I'm all for making the principles more accessible and easy to find. Regarding the placement, I am concerned about the storybook sidebar growing and having to present many different types of content. The structure of these pages is not completely clear to me going forward. For example, this "homepage" is perhaps a more logical place for reading about @KaiVandivier, are you aware of a decision that's been made about these pages? |
|
@cooper-joe That's a valid concern - I don't think the final decision about the documentation future is written down (although it's mentioned in some notes here), but my understanding of the conclusion we reached during the storybook renovation code review is that the storybook would become the primary documentation source; I'll ping @varl to confirm. Getting Started, Recipes, etc. would live in the storybook, but the existing 'home page' would still present an auto-generated API page like it does now (except that it would ideally use React Docgen to generate the API instead of JSDocs so that the source code only needs one kind of comments to document it). There was some talk about eschewing the API page because the storybook can provide that information, which would make the storybook encapsulate everything, but I know @varl likes the API page. I think it's best to have everything in one place, and since the documentation-only pages can go in the storybook but the demos can't go in the doc site, the storybook is the way to go, and it supports docs-only pages pretty well. How does that sound to you @cooper-joe? Maybe this requires a proposal to kind of 'deprecate' the existing home page/doc site to have a definitive decision |
|
@mediremi Linking to the storybook pages has turned out to be a bit complicated by the page being present on both the storybook and the homepage/doc site (which will hopefully be temporary) because they handle links differently. Relative links from markdown on storybook are initiated from an iframe as root (though this is not the case when using MDX). One solution is to link to 'https://ui.dhis2.nu/demo/?path=/docs/slug-slug-slug--page', which will work from the doc site and the storybook, but will require a full refresh on the storybook and won't be very useful during development. Maybe those tradeoffs are okay for now, though. If this were a storybook-only page, linking from one story to another using MDX would not require a full refresh of the page and could use a relative url that works both during development and deployment, so this is more motivation to pursue consolidating UI documentation in the storybook. |
Good point, and thanks for refreshing my memory on the discussions that took place. That does support Storybook being the primary documentation source. I don't think it's necessarily a problem now, I think I'm just wary of that sidebar growing too much. I don't think my concerns warrant blocking this PR. |
There was a problem hiding this comment.
I can't comment on the implementation details, but I generally approve.
One question I do have: are there consequences to the Design System repo? I assume that I'll just keep adding/updating content there as necessary and anything in the principles folder will be pulled in automatically?
|
Thanks Joe! |
I think that could potentially be required in the future (maybe splitting the Principles into folders based on theory/practice, or something) - but I don't think it's necessary yet. It's good to keep that option in mind though if/when the amount of pages in those sections grows. |
There was a problem hiding this comment.
I'm probably not the best person to review this PR. The main point I'm unsure about is how you are producing the files via template strings. We could switch to a slightly more robust AST approach. Having said that, I can't fault the template string approach either, there's nothing very complex going on, so perhaps sticking to a simple approach is best...
Besides that I have placed 2 comments in the code and I think we should address/discuss the error handling point before proceeding.
package.json
Outdated
| @@ -75,6 +75,8 @@ | |||
| "concurrently": "^5.3.0", | |||
| "enzyme": "^3.11.0", | |||
| "enzyme-adapter-react-16": "^1.15.5", | |||
| "lodash": "^4.17.21", | |||
There was a problem hiding this comment.
maybe just add lodash.startcase instead?
yarn add lodash.startcase
and const startCase = require('lodash.startcase');
There was a problem hiding this comment.
Good idea :) after looking into it, that method will be deprecated, but it seems like there's a better way:
if you import or require methods directly, e.g. const throttle = require('lodash/throttle'), only the subset of lodash code your package uses will be bundled in projects that use your package.
| .then(() => | ||
| console.log('Design system files downloaded & processed for storybook') | ||
| ) | ||
| .catch(console.error) |
There was a problem hiding this comment.
Catching the error and logging it to the console implies we only want to notify about this failure. I think for something like this, that does probably make sense: we can still showcase our components without including the design system files. However, this does imply that the storybook demo is still functional with these and I have not verified this.
Also, I think we should probably include some additional information about the context before printing this error to the console, i.e.:
The following error occurred while downloading and processing design system files. The design system will not be included in the demo storybook.
(and then the error)
There was a problem hiding this comment.
That's a good point -- if there's something that could go wrong that would interfere with the rest of the storybook, it would be partially-made story files that could cause run-time errors. In that case, if there's an error, maybe the directory the design system files get copied into should get wiped to delete any malformed files
|
Thanks @HendrikThePendric! Nice tip about the AST processing -- I agree that using a template string is probably good enough for this application, BUT there are some things that could be better about the link transformations and other things related to migrating from GitHub to storybook, and the AST approach might help with that. I'll look into it! I think another improvement for the file outputs is to skip making |
|
Hey @KaiVandivier and @cooper-joe 👋 Fun topic and interesting PR, nice work @KaiVandivier and thanks for digging into this. Over the last 2 years we (me and joe) have talked about merging the design system with the UI repo, and last time I checked that's still the long term goal. Perspectives change over time though, so I think it would be good if we three (@varl, @KaiVandivier, @cooper-joe) have a meeting and discuss the way forward before we merge this. Let's change it to a draft to be on the safe side. |
|
Sounds good 👍 |
|
Hi! Due to a lack of activity on this issue over time (180 days) it seems to be stale. If still relevant, please provide information that moves it forward, e.g. additional information, a pull request with suggested changes, or a reason to keep it open. Any activity will keep it open, otherwise it will be closed automatically in 30 days. Thanks! 🤖 |
|
Automatically closing due to lack of activity. 🤖 |
This adds a script to fetch the 'Principles' pages from the Design System and formats for them for use in the storybook so users can see that info side-by-side with component demos. The fetch script runs before the
build-storybookcommand in thedemoscript, and the fetched files are added to a git-ignored-directory in./docs.To avoid having to download the whole design system and all of its assets, any links to other pages or assets are transformed to external Github links. External links and same-page links are left as-is.