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

Add products route #3

Merged
merged 5 commits into from Mar 11, 2020
Merged

Add products route #3

merged 5 commits into from Mar 11, 2020

Conversation

@van-fs
Copy link
Member

van-fs commented Mar 11, 2020

This PR adds a products route:

  • Products are backed by a static json file in public/data
  • There are two routes added products and products/<id>
  • Lunr provides a lightweight search service

On a design note, I moved the getProducts and getProduct middleware out of ProductController. As I was working, it felt strange to have instance methods inside the ProductController making static calls. I'll submit another PR to refactor the PingController and cleanup if there are no issues with this design change.

@patrick-fs

This comment has been minimized.

Copy link
Member

patrick-fs commented Mar 11, 2020

On a design note, I moved the getProducts and getProduct middleware out of ProductController. As I was working, it felt strange to have instance methods inside the ProductController making static calls.

Please see my suggestion - I think you can refactor stuff out of controller and into a ProductDatabase class that is a client to the ProductController.

Copy link
Member

patrick-fs left a comment

LGTM! I added a couple of Nit's for you to look into.

@van-fs van-fs merged commit 8e70f36 into master Mar 11, 2020
@van-fs van-fs deleted the van/products-route branch Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.