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

Refactoring hard-coded frontend route keywords #4879

Closed
wants to merge 1 commit into from

Conversation

katiefenn
Copy link

issue #4519

  • Added configurable route keywords
  • Replaced instances of hard-coded keywords with config
  • Added keywords to frontend tests stub config

- Added configurable route keywords
- Replaced instances of hard-coded keywords with config
- Added keywords to frontend tests stub config
@ErisDS
Copy link
Member

ErisDS commented Mar 21, 2015

Hey @katiefenn, so sorry that it took me such a long time to get to this. I've both read through the code and taken it for a spin, and it's looking great.

I think keeping it in the config for now is OK as it's not possible to set it through the config.js. Eventually I think this would be set through the database settings or some such, which will mean dynamically loading it, but this PR means we can solve that problem separately.

One super nit-picky comment is that in some places config.routeKeywords gets repeated a lot, it'd be my personal preference to have a slightly shorter name or store a local variable to make it a bit shorter - but I don't have a good suggestion for a better name and it's not really an issue.

That just leaves me to ask if you could please update the commit message to match the preferred format?

@ErisDS
Copy link
Member

ErisDS commented Mar 23, 2015

Hey @katiefenn, I know you've been busy building your new Ghost site, so I have fixed up the commit message and merged this into master manually :)

💃

@ErisDS ErisDS closed this Mar 23, 2015
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.

2 participants