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

ui: Remove storybook, add docfy #9831

Merged
merged 11 commits into from
Mar 8, 2021
Merged

Conversation

johncowen
Copy link
Contributor

Back in #9049 we added storybook with a view to providing an approach to catalog and work on our components in an isolated manner. Whilst storybook has been installed and used in other projects, its never really sat that well with us:

  1. Lots of dependencies that aren't relevant to us elsewhere (react, postcss etc etc). As always with adding dependencies, all of these require time and effort to keep up to date, which would be fine if we were a react project - but we're not.
  2. We installed and used the ember-cli storybook addons, which make it possible to use storybook for ember/glimmer components/projects. Whilst its amazing how far folks have got to providing an ember-like experience, writing stories as markdown, with embedded react, with embedded JSON, with embedded handlebars, with embedded glimmer components felt a little strange. Ideally we would like something that feels more native ember.
  3. There seem to still be a few rough edges on the ember support of storybook, and whilst its amazing that folks have got it integrated to the extent that its at, there are a few things we found tricky and its not massively clear whether this is something to improve from the storybook or the ember side of things.
  4. Having used storybook for other things, depending on what you are doing its relatively straightforwards to add and bend things to your liking, however with the ember support added aswell, knowing where to begin when configuring, adding and tweaking things feels a little opaque.

All in all, whilst it would be great to see storybook in ember/glimmer improving over time it's unfortunately not something that works for us right now. (but we'll be keeping an eye on things as we don't want to rule it out in the future)

To back up a little, a while ago we switched to a nested component structure and started add README files to each components folder (#7448). As they are just plain and simple markdown README files they are completely readable in GitHub, which meant readable docs with no extra effort of providing a way to read them. You can quite easily use GitHub as our documentation site.

The missing feature here is being able to render our Glimmer components live inside the README files.

At the time we just used code blocks so you could at least see the code used to use a component and left this missing feature as a future exercise (at the time storybook was thought to be the potential solution).

Recently we stumbled upon docfy and its ember specific addon and decided to give it a try to see how straightforwards it would be to use it to render the code blocks of our README files as live rendered glimmer components.

After a few lines of docfy configuration and adding a:

```hbs preview-template

to our code blocks, we'd solved our missing feature 🎉

This PR removes storybook and adds docfy and uses docfy to render our existing README files.

This now means we can keep adding README documentation without commiting any specific format or framework. If we eventually move to storybook then fine, or if we just want to remove docfy for whatever reason then fine - we will still have a full set of README files viewable via GitHub.

Additionally this PR uses our <App /> component to provide sidebar navigation for all of our component README files, and adds some ember configuration to keep any docfy/documentation code out of our production build. Whilst this exclusion configuration works fine, we'll probably improve this and externalize it a bit better at a later date (we have quite a bit of 'debug code' now dotted around and it would nice for it to all be in one place).

We also noticed whilst we were doing this that although the older ember documentation for runInDebug states Uses of this method in Ember itself are stripped from the ember.prod.js build., but that is not actually the case 😬 (or at least not in later versions).

I'm not sure exactly when the documentation was updated to remove this statement (or if runInDebug statements were ever stripped from prod builds in the first place), but we'll probably need to alter our approach to debug code in general if we want to avoid debug code being included in the payload of production builds. So that will be a good opportunity to think about how we handle 'debug only code' at a more general/global level.

Lastly the code here is enough to allow us to keep adding more and more documentation as we go, without adding extra weight to our production build. We'll probably keep moving forwards with the docs pages with some styling tweaks etc.

@johncowen johncowen added the theme/ui Anything related to the UI label Feb 25, 2021
@johncowen johncowen requested a review from kaxcode February 25, 2021 14:21
@hashicorp-ci
Copy link
Contributor

🤔 Double check that this PR does not require a changelog entry in the .changelog directory. Reference

Copy link
Contributor

@kaxcode kaxcode left a comment

Choose a reason for hiding this comment

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

Nice cleanup 🔥

@johncowen johncowen merged commit 308e5a4 into master Mar 8, 2021
@johncowen johncowen deleted the ui/feature/docs-improvments branch March 8, 2021 12:22
@hashicorp-ci
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/334156.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants