-
Notifications
You must be signed in to change notification settings - Fork 237
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
Setup router api #1572
Setup router api #1572
Conversation
… Updated filters to match the API changes.
73427af
to
f173fca
Compare
I think we can do better with the name of |
@natcarey changes make sense to me, would you mind updating the changelog in this PR? |
96363e7
to
6c315bf
Compare
CHANGELOG.md
Outdated
@@ -12,7 +12,7 @@ | |||
- Assets and Javascript are served from their location in the app folder rather than being copied to a public folder | |||
- Generated assets are all inside .tmp | |||
- The core prototype-kit files have been moved into the package | |||
- The start script uses the new govuk-prototype-kit cli | |||
- The start script uses the new uk-prototype-kit cli |
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.
typo - govuk
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.
Fixed
@@ -1,6 +1,4 @@ | |||
const express = require('express') | |||
const router = express.Router() | |||
/* eslint-disable-next-line no-unused-vars */ |
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.
is there any way for users to not get these lines, as they probably don't need them and they are a bit daunting
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.
Yeah, I think we should look at the starter kit as a whole and look at this line as part 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.
Just my comment about the name of the cli in the comment within the changelog, otherwise looks good.
CHANGELOG.md
Outdated
@@ -12,7 +12,7 @@ | |||
- Assets and Javascript are served from their location in the app folder rather than being copied to a public folder | |||
- Generated assets are all inside .tmp | |||
- The core prototype-kit files have been moved into the package | |||
- The start script uses the new govuk-prototype-kit cli | |||
- The start script uses the new uk-prototype-kit cli |
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.
Has govuk-prototype-kit been changed to uk-prototype-kit by mistake?
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.
Fixed
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.
looks good thanks!
6c315bf
to
91eed1a
Compare
Allowing the user to create a router.
We explored ideas around more fine grained control instead of providing router functionality but I've reconsidered and think that's better to grow over time alongside providing routers. We can deprecate the routers at a later date if we want to replace them with more fine grained methods.