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

Enhancement/Remove demo components #276

Merged

Conversation

@c-w
Copy link
Member

c-w commented Jul 5, 2019

As discussed in #274, we currently have some code duplication between the demo labeling canvas and the production labeling canvas. This means that new frontend features may have to be implemented twice which slows down the process and may confuse developers.

This pull request refactors the demo pages to use the production labeling canvas. This is achieved by using axios-mock-adapter to intercept the HTTP requests of the production labeling canvas and replacing them with an in-memory implementation. In the future, this same approach can also be leveraged for faking HTTP requests for unit testing the frontend.

@c-w c-w force-pushed the CatalystCode:enhancement/remove-demo-components branch from 10c8ee3 to 4515228 Jul 7, 2019
@Hironsan Hironsan merged commit 1c4894a into doccano:master Jul 9, 2019
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Travis CI - Pull Request Build Passed
Details
@Hironsan

This comment has been minimized.

Copy link
Member

Hironsan commented Jul 9, 2019

Wonderful!

@c-w c-w deleted the CatalystCode:enhancement/remove-demo-components branch Jul 9, 2019
@guillim

This comment has been minimized.

Copy link
Contributor

guillim commented on 4515228 Jul 10, 2019

@c-w is there any chance this bug
Module not found: Error: Can't resolve 'axios-mock-adapter' in '/src/app/server/static/components/demo' was introduce in this commit ?

I have this issue now (using latest version of master branch / 50c7078 ), and debugging the different commits lead me to this one:
commit 61fd677 (Make null checks work with undefined variables) is fine, but commit 4515228 (Use production labeling canvas for demo pages) sounds bugged

This comment has been minimized.

Copy link
Member Author

c-w replied Jul 10, 2019

@guillim Given the /src component in the path, I'm assuming you're using the docker-compose setup? If so, note that we're caching the node_modules directory in a volume so we don't have to reinstall the dependencies on every restart. This pull request added a new dependency so you'll have to force a reinstall of the dependencies in docker-compose via docker-compose down --volumes.

There's probably a better way to solve this (e.g. check last modified time of package.json versus the node_modules folder) but I don't have bandwidth to look into this currently.

This comment has been minimized.

Copy link
Contributor

guillim replied Jul 10, 2019

@c-w Tks ! --volumes was the point I missed, sorry for bothering.
It works like a charm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.