ome / omero-figure

An OMERO.web app for creating Figures from images in OMERO
http://figure.openmicroscopy.org
GNU Affero General Public License v3.0
15 stars 31 forks source link

code layout #178

Closed will-moore closed 7 years ago

will-moore commented 8 years ago

This moves all the source code js and templates files out of the Django app static directory into a new top-level src directory.

The grunt jst and concat tasks compile templates and concatenate js files into omero_figure/static/figure/templates.js and omero_figure/static/figure/figure.js.

All subsequent PRs will need to be on top of this one to avoid conflicts. To test:

NB: removed the build date that is updated with every concat of figure.js to reduce conflicts between PRs.

cc @bramalingam Best test of this PR is to begin a new feature on top of it.

atarkowska commented 8 years ago

templates has to stay under omero_figure otherwise they won't be part of pypi package

"NetworkError: 404 Not Found - https://cowfish.openmicroscopy.org/webtrial/static/figure/templates.js?version=2.0.0"
templat...n=2.0.0
ReferenceError: JST is not defined

roiTemplate: JST["src/templates/modal_dialogs/roi_modal_roi.html"],
atarkowska commented 8 years ago

could you also bump version to 2.0.1-dev?

will-moore commented 8 years ago

I think we decided not to use -dev in version numbers now? cc @jburel @sbesson We discussed proposal to use branches instead of -dev flag.

will-moore commented 8 years ago

@aleksandra-tarkowska The source templates don't need to be in the pypi package. Only the compiled templates.js file does (and it has moved from static/figure/js/templates.js -> static/figure/templates.js).

will-moore commented 8 years ago

Do we bump to version 2.0.1 now or at release time?

atarkowska commented 8 years ago

yes, but templates.js is not there

jburel commented 8 years ago

Could you roll back the license change from c40d049? I had to fix other similar issues in https://github.com/ome/omero-figure/pull/181.

jburel commented 8 years ago

Closing this PR so we can fix the set up first #181

will-moore commented 8 years ago

@bramalingam You want to try starting some work on top of this PR in order to review these changes?

will-moore commented 7 years ago

@bramalingam Now that you're working on top of this branch, please add any comments here about code layout. If you're happy with it, we can merge this then you can go back to working on top of master. Thanks.

will-moore commented 7 years ago

@bramalingam This new code layout is good to merge?

bramalingam commented 7 years ago

Opened a PR on top of this branch, https://github.com/ome/omero-figure/pull/187: Looks good to merge.

will-moore commented 7 years ago

@jburel If you could merge this, then we can rebase & cleanup the other 3 PRs on top of it, thanks.

jburel commented 7 years ago

what was the reason for commenting out the "jasmine" target? if it is no longer needed, remove it. and maybe,in that case, remove the link in readme" why grunt and jasmine Also it will be nice to have a "grunt clean" when we removed the concatenated file from the rep but that can be done in another PR

jburel commented 7 years ago

also "grunt test" in package.json The "test" target does not exist

jburel commented 7 years ago

@will-moore: can you remove the last commit, this is already done in https://github.com/ome/omero-figure/pull/186

will-moore commented 7 years ago

@jburel - If travis fails, can this PR be merged?

jburel commented 7 years ago

mine should be merged tomorrow am (need ola to remove the "red circle" mainly to avoid confusion) then you can merge develop into your PR then travis should be back to green. Need to review your last change

jburel commented 7 years ago

@will-moore: PR has been approved and now merge you should now be able to merge develop and have travis back to green

jburel commented 7 years ago

what about "scripts": { "test": "grunt test" }, in package.json

still also not sure about the link in readme to the blog post since now jasmine is gone

jburel commented 7 years ago

what about: "scripts": { "test": "grunt test" }

also for bootstrap: Code licensed MIT not Apache

jburel commented 7 years ago

Merging so we can move forward the minor cleanup noticed can happen in another PR