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 Django template inheritance from annotation pages #159

Conversation

@c-w
Copy link
Member

c-w commented Apr 13, 2019

This pull request is a follow-up to #148 and moves the annotation pages to Vue single-file components. This change also fully decouples the Vue frontend for the annotation from the Django server by removing all usage of Django block-based template inheritance.

The main approach to removing the Django template inheritance is modeled after the article Extending VueJS Components and leverages the Pug pre-processor for Vue templates.

In the future, a separate refactor could further simplify the approach and remove Pug in favor of splitting the Vue components into smaller independent pieces and using slots for the content composition. This would be cleaner overall solution, but it would also be a much more invasive refactoring. In any case, I believe Pug is a good step in the right direction and will make these future refactorings easier since now all the templating code is in Vue. Having all the templating code in Vue also makes other efforts such as component re-use or testing much simpler. Alternatively, if the project decides that Pug offers value on its own due to its improved terseness over plain HTML templates, other Vue components could also be converted to Pug in the future to make the codebase more uniform.

The Pug approach demonstrated in this pull request can also be extended to the admin pages in Doccano, for example to replace the Django template inheritance on the upload and download screens, but I haven't tackled this yet.

Unfortunately the pull request diff is somewhat large, so please don't hesitate to reach out with questions or comments. However, most of the changes made were very mechanical and so the overall pull request should be low risk. See below for an outline of the refactoring approach. Steps 1-3 below are in commit d5bc9e4. Step 4 is commit bcfaad4.

  1. For each webpack bundle, copy the source Javascript file into the <script> section of a new Vue single file component. Keep all the code associated with creating a new Vue instance in the webpack bundle Javascript file and remove those lines from the Vue single file component so that the file only exports a pure Vue component without any side-effects.

  2. For each Django base template (e.g. annotation_base.html), move all HTML associated with a Vue template into a Pug file. Convert all the HTML to the equivalent Pug code and replace each Django block with a Pug block.

  3. For each Django leaf template (e.g. document_classification.html), move all HTML associated with a Vue template to the <template> section of the appropriate Vue single file component. If the Django template referenced a block, convert the HTML to Pug and extend the appropriate Pug base template.

  4. All the Django leaf templates now only differ in the webpack bundle they include to scaffold the page, so delete them all in favor of a single base template which uses the Django context to configure which webpack bundle should be injected. Update all the views in Django to use the new base template and set the appropriate context variable for the webpack include.

In order to ensure that all Pug templates remain high quality going forward, I've also added a linter for Pug in commit ab473e3.

@Hironsan

This comment has been minimized.

Copy link
Member

Hironsan commented Apr 15, 2019

And It will occur something strange in a sequence labeling project:

image

@c-w c-w force-pushed the CatalystCode:enhancement/remove-django-template-inheritance-from-annotation branch from bcfaad4 to 2bdcada Apr 16, 2019
@c-w

This comment has been minimized.

Copy link
Member Author

c-w commented Apr 16, 2019

@Hironsan Thanks for the initial review. Could you share the data file (or a subset thereof) that exhibits the strange behavior in the sequence labeling project?

@Hironsan

This comment has been minimized.

@c-w

This comment has been minimized.

Copy link
Member Author

c-w commented Apr 16, 2019

@Hironsan Thanks for providing the source file. I was able to reproduce the issue and fixed it in e5fc2db:

Screenshot showing fixed sequence labeling UI

@c-w

This comment has been minimized.

Copy link
Member Author

c-w commented Apr 16, 2019

Based on the work in this pull request, I now also have a branch that moves the admin pages to Vue single-file components (see diff). I will submit these changes once this PR is fully discussed, reviewed and merged.

@c-w c-w force-pushed the CatalystCode:enhancement/remove-django-template-inheritance-from-annotation branch from ac4292b to 88a4826 Apr 17, 2019
@c-w

This comment has been minimized.

Copy link
Member Author

c-w commented Apr 22, 2019

@Hironsan Just following up on this pull request. Do you have any further questions or concerns about this change that I can help address?

@Hironsan Hironsan merged commit fd4910f into doccano:master Apr 25, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@c-w c-w deleted the CatalystCode:enhancement/remove-django-template-inheritance-from-annotation branch Apr 25, 2019
@c-w

This comment has been minimized.

Copy link
Member Author

c-w commented Apr 25, 2019

Thanks for the merge! I've now also submitted the follow-up PR which applies the same refactor to the admin pages.

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