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

Create demo app and move documentation to sub-app #244

Merged
merged 12 commits into from
Oct 13, 2016
Merged

Conversation

edwardhorsford
Copy link
Contributor

This is a newer version of #158.

This PR moves documentation and examples outside of app and in to a new sub-app called docs. This gives us a more consistent place to put documentation, and means our upgrade path is easier - users upgrading via copying app will now be able to get up to date documentation.

This PR also introduces a new documentation 'home' - the intention being we can push this to heroku and have a single place we can point people at to get the kit and docs, etc.

You can play with it here: https://prototype-kit-documentation2.herokuapp.com

Fixes #151

Changes:

  • Move docs and examples in to sub-app /docs/
  • New docs start page which links to everything.
  • Uses github API to get the url to the zip of the latest release - easier than linking to github.
  • Renders markdown files as html - markdown will still work and link in Github though (though we may not need this).
  • Introduces a config var to enable docs or not.
  • Introduces a env var to redirect the root to docs - this means we can deploy to govuk-prototype-kit.herokuapp.com and have the docs be the primary page.
  • Change to npm module nunjucks from express-nunjucks, as it didn't seem to correctly support sub-applications.

Things that aren't finished:

  • The tutorials and examples page is a bit of a mess. There's too many things. Potentially tutorials and examples could be two separate pages, but there's a bit of overlap between them.
  • I think the end navigation should be `Download | Getting started | Tutorials and examples | Github | About - I haven't done this yet though.

NB: I'd expect this to be a major version change.

@edwardhorsford edwardhorsford changed the title Create demo app and move documentation to sub-ab Create demo app and move documentation to sub-app Aug 22, 2016
<li><a href="/docs/install">Install</a></li>
<li><a href="/docs/tutorials-and-examples">Tutorials and examples</a></li>
<li><a href="/docs/about">About</a></li>
<li><a href="https://github.com/alphagov/govuk_prototype_kit">Github</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Github > GitHub

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@joelanman
Copy link
Contributor

nice! thanks for all this work

Just going through it - but a quick note: I think the copy under the headings on the home page could be clearer - what do you think about getting a content person on it?

@extend .heading-small;
}

code {
Copy link
Contributor

@joelanman joelanman Aug 22, 2016

Choose a reason for hiding this comment

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

can't remember who wrote this originally, but why isnt the code block just @extend .code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, but you need the other things for it to display correctly I think. I'm still not happy with the code styling though.

@joelanman
Copy link
Contributor

In the existing docs, you can jump straight to Run the kit, which we've seen in training to be useful. Don't think you can jump to that in the new docs, it's just the last page of "Install the kit". Do you think we can link to it from somewhere?

});

router.get('/install', function (req, res) {
console.log('hi');
Copy link
Contributor

Choose a reason for hiding this comment

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

delete?

@joelanman
Copy link
Contributor

look like it all needs to go through the linter - there are some missing 'var' declarations, semi-colons to be removed. Try a rebase, then npm test

res.render("install_template", {"document": html});
});

// Examples - exampes post here
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

@joelanman
Copy link
Contributor

joelanman commented Aug 22, 2016

can docs/documentation_routes.js just be docs/routes.js?

@joelanman
Copy link
Contributor

on the / index page, we should probably link to the new docs app on heroku, rather than docs on github

@joelanman
Copy link
Contributor

maybe we should use Express 4 sub apps, as they seem designed for this situation: http://www.therightcode.net/differences-between-express-3-and-express-4/

exports.matchRoutes = function(req, res) {

var path = (req.params[0]);
console.log('path is', path);
Copy link
Contributor

Choose a reason for hiding this comment

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

delete?

@gemmaleigh
Copy link
Contributor

Hi @edwardhorsford to try and help out a little here before you try to rebase against master - I've added a commit where I add standard as a dependency and start linting the files in this codebase.

Hopefully it will help make the rebase with master a little less painful. That commit can always be removed later.

To continue with linting, run npm test, hope that helps!

request = require('sync-request')

// Variables
var releaseUrl = null

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

these comments belong to the basicAuth function, so they need to be moved back to being above it

@edwardhorsford
Copy link
Contributor Author

@joelanman I'm intentionally giving the files distinctive names so that people don't accidentally start editing the wrong one when using quick-search.

Not sure about the differences between the different apps - but the current ones seem to work ok to me. What are the benefits of switching?

@joelanman
Copy link
Contributor

@edwardhorsford I'm not sure how scalable it is to avoid duplicate filenames - for example we already have multiple index.html. In quick search you can include the name of the parent folder (giving you the same outcome as the longer name), and it shows the parent folder in the search too.

There's value in using the Express 4 app approach because thats the standard documented way to do it - development in the future would be easier as people can consult those docs, and other modules might expect that standard approach. But if you're happy this approach works we could always cross that bridge if we came to it.

@edwardhorsford
Copy link
Contributor Author

I'm not sure what problem there is for it to have a unique name in this case. It's a mistake I personally made multiple times. The current name is descriptive for what the file is. I'm not suggesting a blanket rule of avoiding duplicates, but having of having a descriptive name in this case for a file users will likely be editing frequently.

I'm not familiar with the other approach for apps, and the current one works. If you'd like to tackle it, great, else perhaps we release and raise an issue to consider it later?

edwardhorsford and others added 6 commits October 13, 2016 11:07
This commit can be removed later as these files have already been
linted on master, but it may well help with the rebase.

I’ve made a start on linting here, to continue with linting run `npm
test`, there are still errors in server.js. utils.js and
documentation_routes.js.
Robin Whittleton added 2 commits October 13, 2016 11:33
- Docs should have its own JS file (which now includes the latest from toolkit)
- Docs images should be copied into their own folder in /public
@robinwhittleton
Copy link
Contributor

OK, @gemmaleigh and I have just reviewed this and it looks good. We’ve made a few more changes after @edwardhorsford’s work to sort out the docs assets. Happy to merge, and we can deal with any docs updates after that.

@robinwhittleton robinwhittleton merged commit 3ed3730 into master Oct 13, 2016
@robinwhittleton robinwhittleton deleted the docs_app branch October 13, 2016 11:09
robinwhittleton pushed a commit that referenced this pull request Oct 13, 2016
Breaking changes:

- #244 Migrate documentation into a seperate application

All changes:

- Bump all GOV.UK assets to their latest versions
- Remove duplicate GOV.UK assets copied to the app
- #241 Warn against using the prototype kit to build production services
- #268 Automatically keep the latest release branch up to date. This can be used to update the kit
- #270 Add a new stylesheet for the unbranded layout to fix font issues
- #257 Make CSS output easier to debug (with sourcemaps)
- #237 Make links with role="button" behave like buttons
- #224 Lint the prototype kit’s codebase using Standard. This only applies to the kit’s codebase - there’s no requirement for your app to meet this
robinwhittleton pushed a commit that referenced this pull request Oct 13, 2016
Breaking changes:

- #244 Migrate documentation into a separate application

All changes:

- Bump all GOV.UK assets to their latest versions
- Remove duplicate GOV.UK assets copied to the app
- #241 Warn against using the prototype kit to build production services
- #268 Automatically keep the latest release branch up to date. This can be used to update the kit
- #270 Add a new stylesheet for the unbranded layout to fix font issues
- #257 Make CSS output easier to debug (with sourcemaps)
- #237 Make links with role="button" behave like buttons
- #224 Lint the prototype kit’s codebase using Standard. This only applies to the kit’s codebase - there’s no requirement for your app to meet this
@robinwhittleton robinwhittleton mentioned this pull request Oct 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants