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

Replace JQuery+HTML by Vue.JS #245

Merged
merged 7 commits into from
Feb 1, 2019
Merged

Replace JQuery+HTML by Vue.JS #245

merged 7 commits into from
Feb 1, 2019

Conversation

kinow
Copy link
Collaborator

@kinow kinow commented Jan 26, 2019

This removes:

  • annif.js
  • base.html template

Then adds a <script> for the Vue.JS CDN (simplest form to get started with Vue.JS). And finally replaces the old logic from the JQuery APP by a VueJS with three components. The components are not really necessary.

We could have the same with a simple application, but it would be a bit harder to see what was doing what. Also, this serves as base for further enhancements.

Here's what it looks like running.

success

If the VueJS developer bar is installed in Firefox, it's possible to see the state of the application.

success-vuebar

Also added some validation. I followed the installation instructions up to the point of having TF-IDF English. Analyzing text with other projects resulted in a server error not displayed to the user. So added a simple validation to display an error.

ajax-server-error

Likewise, in case no results are returned after the analysis, an alert is displayed.

no-results

And in case you do not select a project, or do not enter the text, another alert is displayed.

validation

It would be much simpler to implement this with Vuex, but that's overcomplicating for now I believe. But in case using VueJS sounds like a good idea, before extending its use over the admin pages (#24 ) it would be a good idea to update this app, to use vue-cli, vuex, etc (which takes much longer, but well worth).

Cheers
Bruno

@kinow
Copy link
Collaborator Author

kinow commented Jan 26, 2019

Oh yeah, and everything worked following the documentation! Got some Asimov text annotated (correctly I believe, robots right at the top, for an excerpt from I, Robot I think).

@codecov
Copy link

codecov bot commented Jan 26, 2019

Codecov Report

Merging #245 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #245   +/-   ##
======================================
  Coverage    99.3%   99.3%           
======================================
  Files          51      51           
  Lines        2432    2432           
======================================
  Hits         2415    2415           
  Misses         17      17

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7c532f...df13ee1. Read the comment docs.

@osma
Copy link
Member

osma commented Jan 26, 2019

Great, thanks a lot! Indeed Vue is probably a better basis for further development of the UI. Will do more thorough review next week!

@osma
Copy link
Member

osma commented Jan 28, 2019

Found the time to look at this in more detail.

Observation: this isn't (currently) really replacing jQuery with VueJS, since the requests to the REST API are still done via jQuery as well as some of the form manipulation. I'm not very familiar with what Vue can and cannot do (never really used it myself), but I think it would be desirable to try to build on just one JS framework instead of having a mixture of two, WDYT? Vue itself apparently cannot perform HTTP requests, but there's vue-resource for that.

I also wonder whether bootstrap-vue fits into this somehow or whether it's better to keep Bootstrap and VueJS as separate dependencies?

Right now Bootstrap and jQuery are bundled with the Annif codebase, but you chose to load VueJS from a CDN. I have no strong opinion on which way is better but I think this should be done consistently.

Personally I don't have any big plans for the UI - so far it has been enough to have a form for testing where it's easy to paste text from elsewhere. But moving into the direction of an admin API (#24) - perhaps similar to the Fuseki2 web UI - would be nice and this seems like a sensible first step in that direction.

All in all I'm positive to this and if we can agree on the direction (e.g. the thoughts/questions above) and you feel that the code is ready for merging, I'm happy to pull it in.

@osma osma added this to the Short term milestone Jan 28, 2019
@kinow
Copy link
Collaborator Author

kinow commented Jan 29, 2019

Observation: this isn't (currently) really replacing jQuery with VueJS, since the requests to the REST API are still done via jQuery as well as some of the form manipulation.

Oh, you are right @osma . Good point. We are replacing the part that renders the page and tries to react to user changes (that's where vuejs is actually useful, in creating a reactive app). Normally vue is paired with axios. I only avoided using it in order to provide the smallest example with vuejs possible. And as JQuery was there and it had some methods for ajax and locating elements in the UI, and ended up using it.

I'm not very familiar with what Vue can and cannot do (never really used it myself), but I think it would be desirable to try to build on just one JS framework instead of having a mixture of two, WDYT? Vue itself apparently cannot perform HTTP requests, but there's vue-resource for that.

I think you are right. But I think axios would be a best option due to the examples/tutorials with vue + axios (e.g. https://vuejs.org/v2/cookbook/using-axios-to-consume-apis.html).

@kinow
Copy link
Collaborator Author

kinow commented Jan 29, 2019

I also wonder whether bootstrap-vue fits into this somehow or whether it's better to keep Bootstrap and VueJS as separate dependencies?

Right now Bootstrap and jQuery are bundled with the Annif codebase, but you chose to load VueJS from a CDN. I have no strong opinion on which way is better but I think this should be done consistently.

Right. For this case, we probably need to go with vue-cli or webpack, to have the .vue components, stylesheet preprocessor, transpilers, etc.

@kinow
Copy link
Collaborator Author

kinow commented Jan 29, 2019

Personally I don't have any big plans for the UI - so far it has been enough to have a form for testing where it's easy to paste text from elsewhere. But moving into the direction of an admin API (#24) - perhaps similar to the Fuseki2 web UI - would be nice and this seems like a sensible first step in that direction.

All in all I'm positive to this and if we can agree on the direction (e.g. the thoughts/questions above) and you feel that the code is ready for merging, I'm happy to pull it in.

This pull request only provides a Vue.js app, where Vue is taking care of reactivity, components... we could replace the JQuery $("...") calls to create elements and locate elements by templates and vue/DOM manipulation.

But to completely ditch JQuery and use something like Axios or vue-resource, and to have bootstrap + vue (which I'm using in another project), or even something like vuetify (using for yet another project), we would have to either close this one and start a larger pull request (which will take a bit longer to implement), or merge this one and work on the other pull request, re-using parts of what was done here.

All in all, I think it really depends on whether you prefer to have a Vue app, or if JQuery+HTML would be enough for the admin interface #24 too... it could be an overkill to use Vue if that interface will have just one page perhaps.

@osma
Copy link
Member

osma commented Jan 29, 2019

Thanks for your thoughts @kinow! I think it makes sense to do small incremental changes, rather than try to overhaul everything at once. So moving from jQuery + Bootstrap to VueJS + jQuery + Bootstrap is a good first step. Later we may want to replace jQuery with Axios or even just the native Fetch API as was suggested by @natlibfi-arlehiko in another discussion.

Before merging this PR I would simply like to bundle the VueJS dependency with the Annif codebase instead of relying on a CDN. The simplest way I can think of is to store what the CDN returns into a file, just like we do for jQuery and Bootstrap. Does that make sense?

CDNs have their benefits for public-facing web sites, but I think that in most cases this UI will be used from localhost and thus loading the dependencies (JS and CSS) from localhost too makes sense. It also makes it possible to use the Annif web UI without an internet connection. I've done Annif development on a plane occasionally...

@kinow
Copy link
Collaborator Author

kinow commented Jan 29, 2019

Hi @osma,

Sure, grabbing the CDN version, or this link for the production version: https://vuejs.org/js/vue.min.js (minified).

Will update the pull request tomorrow or Friday (few meetings tomorrow and Thu). Or feel free to update this PR if you prefer to merge it sooner.

@osma osma modified the milestones: Short term, v0.39 Jan 29, 2019
@osma
Copy link
Member

osma commented Jan 29, 2019

Great @kinow ! I'm probably working on other stuff (mainly Vowpal Wabbit related) for version 0.39. I'll let you update the PR, then it can go in too.

@kinow
Copy link
Collaborator Author

kinow commented Jan 29, 2019

Great! And first time I ever heard about Vowpal Wabbit! Got to read about it now. Thanks @osma !

@kinow
Copy link
Collaborator Author

kinow commented Jan 31, 2019

Rebased branch, and also added vue.min.js in the static resources within Annif.

image

@osma
Copy link
Member

osma commented Feb 1, 2019

Excellent!

While testing this I noticed that the new UI displays a generic error message regardless of what went wrong. But the REST API already provides structured Problem JSON error messages (see #27) which have a detail field with more information. It would be great to display that information to the user as well. Of course it's still possible that something goes wrong in a deeper way (server crash, network timeout...) so that no Problem JSON response is available so this should be done carefully.

@osma osma merged commit edcef39 into NatLibFi:master Feb 1, 2019
@osma osma mentioned this pull request Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants