intermine / im-tables

Dynamic Result Table Library
Other
9 stars 27 forks source link

Review project build instructions #203

Closed jshreyans closed 4 years ago

jshreyans commented 4 years ago

Related to #182. I have been trying to set the project up for a while now but every time I get stuck on what instructions to follow. Here are the steps I used:

heralden commented 4 years ago

There are definitely holes in the documentation, and we could make it much easier for contributors.

The workflow I use is npm run dev, then open http://localhost:9000/test/test-load-from-global.html to get a running im-tables. This requires having a local testmine running, which I don't think is a good idea to push onto all contributors.

My suggestion is to create an index.html in the root of the project, then edit the gruntfile to print Open http://localhost:9000 after building when serving. The HTML should be similar to test/test-load-from-global.html, except you could have the contents of test/bundles/umd-consume-global.js as inline JS, and referencing https://www.flymine.org/flymine/service as the service root, instead of the local intermine-demo.

You'll also need a different query which fits Flymine's data model. Here's the complete JS you could use.

var selector = '#demo';
var service  = {root: 'http://www.flymine.org/flymine/service/'};
var query    = {
  "from": "Gene",
  "select": [
    "secondaryIdentifier",
    "symbol",
    "primaryIdentifier",
    "organism.name"
  ],
  "orderBy": [
    {
      "path": "secondaryIdentifier",
      "direction": "ASC"
    }
  ]
};

imtables.configure({
    TableCell: {
      PreviewTrigger: 'hover',
      IndicateOffHostLinks: false
    }
});

imtables.loadTable(
  selector, // Can also be an element, or a jQuery object.
  {"start":0,"size":25}, // May be null
  {service: service, query: query} // May be an imjs.Query
).then(
  function (table) { console.log('Table loaded', table); },
  function (error) { console.error('Could not load table', error); }
);

What do you think @yochannah? Any suggestions for improvement?

jshreyans commented 4 years ago

The workflow I use is npm run dev, then open http://localhost:9000/test/test-load-from-global.html to get a running im-tables.

Can we include this in the documentation for now then?

My suggestion is to create an index.html in the root of the project, then edit the gruntfile to print Open http://localhost:9000 after building when serving.

Yes, this does sound like a good idea. That way, contributors can at least get an instance of im-tables running without any issues.

yochannah commented 4 years ago

@jshreyans - would you feel up to making a PR to fix this? :)

jshreyans commented 4 years ago

@yochannah Sure! I'll start making changes asap

heralden commented 4 years ago

@jshreyans I noticed one thing when testing https://github.com/intermine/im-tables/pull/204 which we should probably address before closing this issue.

The grunt task started by npm run dev runs all the tests whenever it detects a change in the source files. The problem with this is that the tests depend on the local testmine running, and will fail otherwise (rewriting the tests to use something else than a local testmine is far too much work to be considered).

I suggest we remove the testing functionality out of the grunt task, and make sure that there exist a different command (perhaps npm run test) which also supports running in watch mode, meaning it should rerun tests when source files change. We might have to update the documentation as well, if this command isn't mentioned.

The npm run dev command also runs a time-consuming run:bundle_test_indices task, which I don't think is necessary if we're not going to run the tests for this command.

jshreyans commented 4 years ago

@uosl So these are the changes that need to made:

I can make changes to the docs within #206 and make a separate PR for the other 2 changes. Does that sound fine?

jshreyans commented 4 years ago

The npm run dev command also runs a time-consuming run:bundle_test_indices task, which I don't think is necessary if we're not going to run the tests for this command.

That sounds right. I'll make sure to include this.

heralden commented 4 years ago

Sounds good, go ahead with the gruntfile PR! I'll remind you if anything is left out. [=