-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add dist folder to .gitignore
test this please |
.travis.yml
Outdated
@@ -7,6 +7,8 @@ branches: | |||
- master | |||
os: | |||
- linux | |||
before_install: | |||
- yarn global install -g grunt-cli |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you put this in dev dependencies instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
dist/ folder is present in most grafana projects. Probably, there is a reason for that. https://github.com/grafana/clock-panel |
You can find the explanation in the dev guide |
src/main.js
Outdated
import { PanelCtrl } from 'app/plugins/sdk'; | ||
import moment from 'moment'; | ||
import axios from './external/axios.min.js'; | ||
import go from './external/go.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use npm packages (go and axios)
@Julien-Molina perhaps the dist folder should be updated by travis (deployment job)... |
src/main.js
Outdated
@@ -0,0 +1,145 @@ | |||
import { PanelCtrl } from 'app/plugins/sdk'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong file name:
should be AC2Ctrl.js instead of main.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
No tests ? Grafana should have guidelines about testing their plugins |
resolves #1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- add some unit tests (at least the non ui parts)
This project contains some test skeleton.
This link is also interesting in typescript
package.json
Outdated
"license": "Apache", | ||
"repository": "https://github.com/xcomponent/grafana-plugin-ac2", | ||
"devDependencies": { | ||
"@types/angular": "^1.5.8", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove @types/angular and @types/lodash because you have no dependency with angular and lodash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or add the right dependencies...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@Julien-Molina grafana is based on angular. You should do a quick review too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every conversation is not resolved, especially the module.ts file.
Did you test the extension compilation with webpack ? I am not 100% sure of the webpack configuration with typescript
src/ac2-map.ts
Outdated
this.diagram.nodeTemplate = this.getNodeTemplate(); | ||
this.diagram.groupTemplate = this.getGroupTemplate() | ||
this.diagram.linkTemplate = this.getLinkTemplate(); | ||
const $ = go.GraphObject.make; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract the $ expression as a class constant. So you do not have to provide the $ argument to each function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
No description provided.