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

[OC-60] Oc client browser #534

Merged
merged 20 commits into from
Jul 11, 2017
Merged

[OC-60] Oc client browser #534

merged 20 commits into from
Jul 11, 2017

Conversation

nickbalestra
Copy link
Contributor

@nickbalestra nickbalestra commented Jul 8, 2017

Blocking PR that need to be published:

Copy link
Member

@matteofigus matteofigus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It generally looks good, but as discussed, there is still a missing part in the express' static redirector that relies on the physical file being there - which needs to be updated to use both the min + map files from the dependency

@@ -3,4 +3,4 @@ script.
oc.conf = oc.conf || {};
oc.conf.templates = oc.conf.templates || [];
oc.conf.templates = oc.conf.templates.concat(!{JSON.stringify(templates)});
script(src=staticPath+"src/oc-client.min.js", type="text/javascript")
script(src="https://unpkg.com/oc-client-browser@1.0.0/dist/oc-client.min.js", type="text/javascript")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should update the oc's build script so that this version (1.0.0) comes from a require('oc-client-browser/package').version.

Copy link
Contributor Author

@nickbalestra nickbalestra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of minor notes

appveyor.yml Outdated
@@ -24,4 +24,4 @@ install:
build: off

test_script:
- cmd: grunt test
- cmd: grunt
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If appveyor rely on npm test by default, then this could also be cleaned up further

tasks/build.js Outdated
grunt.log[err ? 'error' : 'ok'](
err ? err : 'Client has been built and packaged'
const done = this.async();
const version = ocClientBrowser.version;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

version is not needed anymore here

@matteofigus
Copy link
Member

Fixes #438

@nickbalestra nickbalestra merged commit d609b22 into master Jul 11, 2017
@nickbalestra nickbalestra deleted the oc-client-browser branch July 11, 2017 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants