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

Moves 'web' target to the gulpfile. #8288

Merged
merged 1 commit into from
Apr 14, 2017

Conversation

yurydelendik
Copy link
Contributor

No description provided.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

r=me, with the comments addressed.

gulpfile.js Outdated
.pipe(gulp.dest(GH_PAGES_DIR + EXTENSION_SRC_DIR + 'chromium/')),
gulp.src('test/features/**/*', {base: 'test/'})
.pipe(gulp.dest(GH_PAGES_DIR)),
gulp.src(JSDOC_DIR + '**/*', {base: JSDOC_DIR})
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Apr 14, 2017

Choose a reason for hiding this comment

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

I was puzzled as to why this folder, https://github.com/mozilla/pdf.js/tree/gh-pages/api/draft, wasn't copied over when testing locally.

It seems to me that we should add var JSDOC_BUILD_DIR = BUILD_DIR + 'jsdoc/'; to the other constants at the top of the file, and then use that here instead.
Bonus points if this new constant is also used at https://github.com/mozilla/pdf.js/blob/master/gulpfile.js#L932 as well :-)

gulpfile.js Outdated
FIREFOX_BUILD_DIR + '*.rdf'])
.pipe(gulp.dest(GH_PAGES_DIR + EXTENSION_SRC_DIR + 'firefox/')),
gulp.src([CHROME_BUILD_DIR + '*.crx',
FIREFOX_BUILD_DIR + '*.rdf'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that you're just moving the old code over, but after looking at this line:
Why are we copying files, relevant to the FIREFOX addon, into a folder that ought to contain the CHROMIUM extension!?

gulpfile.js Outdated
spawnSync('git', ['remote', 'add', 'origin', REPO], {cwd: GH_PAGES_DIR});
spawnSync('git', ['add', '-A'], {cwd: GH_PAGES_DIR});
spawnSync('git', [
'commit', '-am', 'gh-pages site created via make.js script',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Replace make.js with gulpfile.js here.

gulpfile.js Outdated
@@ -48,11 +50,15 @@ var COMPONENTS_DIR = BUILD_DIR + 'components/';
var SINGLE_FILE_DIR = BUILD_DIR + 'singlefile/';
var MINIFIED_DIR = BUILD_DIR + 'minified/';
var FIREFOX_BUILD_DIR = BUILD_DIR + 'firefox/';
var CHROME_BUILD_DIR = BUILD_DIR + '/chromium/';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: For consistency with the other lines, the first slash can be removed.

gulpfile.js Outdated
@@ -48,11 +50,15 @@ var COMPONENTS_DIR = BUILD_DIR + 'components/';
var SINGLE_FILE_DIR = BUILD_DIR + 'singlefile/';
var MINIFIED_DIR = BUILD_DIR + 'minified/';
var FIREFOX_BUILD_DIR = BUILD_DIR + 'firefox/';
var CHROME_BUILD_DIR = BUILD_DIR + '/chromium/';
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the extra slash before chromium for consistency with the other variables above it.

gulpfile.js Outdated
FIREFOX_BUILD_DIR + '*.rdf'])
.pipe(gulp.dest(GH_PAGES_DIR + EXTENSION_SRC_DIR + 'firefox/')),
gulp.src([CHROME_BUILD_DIR + '*.crx',
FIREFOX_BUILD_DIR + '*.rdf'])
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not look right. Why would the Chome extension folder need to contain the Firefox .rdf file?

gulpfile.js Outdated
});

gulp.task('wintersmith', ['gh-pages-prepare'], function (done) {
mkdirp.sync(GH_PAGES_DIR);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this already exist because gh-pages-prepare is a prerequisite here?

@timvandermeij timvandermeij merged commit 27c3c33 into mozilla:master Apr 14, 2017
@timvandermeij
Copy link
Contributor

Nice work, thanks!

@yurydelendik
Copy link
Contributor Author

/botio-windows preview

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants