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

Vue integration, bibtex replacement #4978

Merged
merged 61 commits into from Nov 22, 2017

Conversation

@dannon
Copy link
Member

dannon commented Nov 10, 2017

The original plan was to use the citations bugfix/overhaul as a simple vehicle to show off the Vue integration, but this includes quite a few more notable bits from the journey.

I'll elaborate more, and plan to write guidelines for best practices, but in short this pr:

  1. Adds Vue
  2. Adds build support for Vue Single File Components (SFCs)
  3. Cherrypicks @anuprulez work from the demo Vue admin/userkeys display
  4. Updates that to a single file component, so people can see the transition
  5. Adds support for using dependencies straight out of node_modules. This WILL NOT WORK for stuff require()d, but for components which are 100% bundled (as should be everything new), it's way cleaner and easier, and the right way to do it.
  6. Should fix #4906; replaces the bibtex lib with two separate components that are much more flexible and extensible.
  7. Adds axios (https://github.com/axios/axios) as an HTTP client. It's really, really nice.

Neither of these displays really benefit from vuex, but I'm going to try to work an example of that in as well (to keep it simple, will pursue Vuex in a separate PR)

  1. Adds two new application entrypoints, removing old 'require'-based entries for both the masthead and workflow editor. This ideally would have been a separate PR since it's a pretty big step forward, but it was a requirement for getting citations.vue working correctly. (basically, I managed to pick the most entangled component ever to use as a vue conversion example -- d'oh). This dramatically cuts down on the number of files fetched on older pages.

Workflow editor load times, before, then after. Similar performance increases for data libraries, trackster, and circster.

image
image

  1. Bundling of libraries, trackster, circster as well as the aforementioned workflow editor, with similar performance gains. Libraries got cut down by about 80 requests, halving the time it takes to load.

@dannon dannon added the status/WIP label Nov 10, 2017

@dannon dannon added the area/UI-UX label Nov 10, 2017

@dannon dannon changed the title Vue integration, bibtex replacement work as an example Vue integration, bibtex replacement Nov 10, 2017

// TODO refactor into a reusable Vue component display mechanism.
var z = document.createElement('div');
this.page.display(z);
new Vue({

This comment has been minimized.

Copy link
@jxtx

jxtx Nov 10, 2017

Contributor

I think you can simplify this to

new Vue(UserAPIKeys).$mount(z)

Which will make it easier to generalize later because you can just pass the component in, no need for template.

This comment has been minimized.

Copy link
@dannon

dannon Nov 10, 2017

Author Member

Nice, trying that out now.

This comment has been minimized.

Copy link
@dannon

dannon Nov 10, 2017

Author Member

@jxtx Works great, thanks!

@anuprulez

This comment has been minimized.

Copy link
Member

anuprulez commented Nov 21, 2017

@dannon I pulled this branch and tried to create a workflow but could not in Mozilla (57.0 (64-bit)) as well as in Chrome (Version 62.0.3202.89). I am able to open individual tools but not able to add any to a workflow. I cloned the codebase from:

git clone https://github.com/dannon/galaxy --branch bibtex-replacement-lib

and it is up to date. Please ignore if you can create a workflow or this issue is unrelated. Thank you!

Mozilla:

moz

Chrome:

chr

Update: Now I am doing make client and would check again. I did not do it before. Let's see...

@dannon

This comment has been minimized.

Copy link
Member Author

dannon commented Nov 21, 2017

@anuprulez I had left out a bit of the refactoring related to the workflow globals handling, thanks! Works for me with the latest change.

jmchilton and others added some commits Nov 21, 2017

Add a tag to formatted references in citations.vue.
No style associated with it yet but it makes it easier to identify for tours and tests.
@jxtx

This comment has been minimized.

Copy link
Contributor

jxtx commented Nov 21, 2017

So, this is basically the opposite of addressing @jmchilton 's concern about complexity and additional libraries, but rather than making a comprehensive bib renderer ourselves can we use citation-js or something similar that uses the citation style language standard?

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented Nov 21, 2017

Just to clarify - I was opposed to adding additional dependencies without getting some clear benefit. Not actually building a bibtex renderer ourselves would be a huge benefit (see commit message here for instance).

@jxtx

This comment has been minimized.

Copy link
Contributor

jxtx commented Nov 21, 2017

Fair enough! Let's get out of this game.

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented Nov 21, 2017

Let's get out of this game.

... as part of a subsequent PR so we don't block Vue even further on the citations point... right @jxtx?

@jxtx

This comment has been minimized.

Copy link
Contributor

jxtx commented Nov 21, 2017

Mais oui!

@martenson

This comment has been minimized.

Copy link
Member

martenson commented Nov 21, 2017

  1. There is a horizontal gap before the citation is shown.

screenshot 2017-11-21 11 56 59

  1. Also the 'cite us' text is new, right? I am not a fan (it is probably not for me to decide though), however due to its color it is the most eye-catching thing on the whole toolform page which I see as problematic.

  2. Can we get rid of the console logs from vue? Also it seems we need to adjust the release process to accommodate for switching vue to production.

vue.js:8260 Download the Vue Devtools extension for a better development experience:
https://github.com/vuejs/vue-devtools

12:07:28.891 vue.js:8270 You are running Vue in development mode.
Make sure to turn on production mode when deploying for production.
See more tips at https://vuejs.org/guide/deployment.html
12:07:29.195 
  1. the vue devtools do not seem to work with our vue (probably not a problem)

  2. after running make client on this branch I have a change in untracked file static/scripts/libs/bibtex.js

  3. after clicking 'visualize in charts' on a dataset:

Uncaught TypeError: Utils.get is not a function
    at app.js:12
    at Object.execCb (require.js?v=1511280824:1)
    at b.check (require.js?v=1511280824:1)
    at b.<anonymous> (require.js?v=1511280824:1)
    at require.js?v=1511280824:1
    at require.js?v=1511280824:1
    at each (require.js?v=1511280824:1)
    at b.emit (require.js?v=1511280824:1)
    at b.check (require.js?v=1511280824:1)
    at b.enable (require.js?v=1511280824:1)
var citationInstance = Vue.extend(Citations);
var vm = document.createElement("div");
this.page.display(vm);
new citationInstance({ propsData: { id: QueryStringParsing.get("id"), source: "histories" } }).$mount(vm);

This comment has been minimized.

Copy link
@martenson

martenson Nov 21, 2017

Member

this spacing seems to be different from our standard

This comment has been minimized.

Copy link
@dannon

dannon Nov 21, 2017

Author Member

It is whatever client-format did.

This comment has been minimized.

Copy link
@martenson

martenson Nov 21, 2017

Member

I thought we abandoned extra spaces with regards to parens.

@jmchilton

This comment has been minimized.

Copy link
Member

jmchilton commented Nov 21, 2017

The cite is text used to only be in the history wide view - where it looks better and makes more sense IMO. There you are summarizing a whole analysis and would expect many more citations and aren’t trying to take focus away from the tool. I wouldn’t include it in the tool form this way - I agree completely.

@dannon

This comment has been minimized.

Copy link
Member Author

dannon commented Nov 21, 2017

Yeah, the text isn't new, but it seemed like a good idea this morning to standardize the (now trimmed down) info message. Seems less like a good idea in retrospect; I'll add the toggle back in.

@dannon

This comment has been minimized.

Copy link
Member Author

dannon commented Nov 21, 2017

Ok.

  • Popped the message back off the toolform; only shows in the 'history citations' view, adjusted the structure slightly.
  • Vue debug text will go away after more work building an actual production bundle at release time, probably not 18.01
  • Vue devtools are working fine for me in firefox, not sure what's up on your end there.
  • The bibtex.js file is because of a (now) untracked previously bibtex.js in your client libs.
  • looking at charts now...
@dannon

This comment has been minimized.

Copy link
Member Author

dannon commented Nov 21, 2017

The charts issue is unrelated to this PR (broken in current -dev). I'll follow up with a fix separately.

@martenson

This comment has been minimized.

Copy link
Member

martenson commented Nov 21, 2017

@dannon ad charts: I did only check against 17.09, we are fast in breaking! :)

@dannon

This comment has been minimized.

Copy link
Member Author

dannon commented Nov 21, 2017

@martenson Yeah, it's related to the other massive client changes we already did this cycle :)

@anatskiy

This comment has been minimized.

Copy link
Contributor

anatskiy commented Nov 21, 2017

Great work! I didn't notice any issues :)

@guerler guerler merged commit 4d416dc into galaxyproject:dev Nov 22, 2017

7 checks passed

api test Build finished. 317 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 162 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 58 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript 1 new alert
Details
selenium test Build finished. 100 tests run, 1 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details

@guerler guerler referenced this pull request Nov 24, 2017

Merged

Restore admin form routes #5065

@dannon dannon referenced this pull request Nov 25, 2017

Closed

Charts Broken on dev #5067

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