search

alliance-pcsg / primo-explore-custom-actions

:hammer_and_wrench: easily add custom actions to primo-explore's actions menu.
MIT License
15 stars 7 forks source link

Feature/tests and fulldisplay bugfix #13

Closed etgrieco closed 6 years ago

etgrieco commented 6 years ago

Unit-testing coverage and bugfix for https://github.com/alliance-pcsg/primo-explore-custom-actions/issues/12

etgrieco commented 6 years ago

Also, https://github.com/alliance-pcsg/primo-explore-custom-actions/issues/11

thatbudakguy commented 6 years ago

It seems like this gulpfile is written for gulp < 4.0, but we have 4 specified in package.json. I'm getting AssertionErrors about the task definitions; maybe because they don't use gulp.series() and gulp.parallel()?

Test tasks run fine and tests pass if gulp isn't involved, but the npm scripts that try to run gulp are failing for me.

I could change the gulpfile syntax since I'm using 4 on another similar plugin, but wanted to check that that's actually what's going on.

etgrieco commented 6 years ago

@thatbudakguy thanks for pointing this out. I believe what was happening was my system gulp was being used because the test command wasn't properly prepended by yarn (edit: actually, yarn commands will run package version). What we can do is downgrade the package back to version 3 or upgrade the gulpfile syntax to 4. Up to you.

I was able to successfully run things by downgrading gulp to: "gulp": "^3.9.1". Just pushed those changes. Also removed babel-loader as it wasn't necessary.

If you are interested in using babel alone instead of gulp, we have successfully done so on many of our customizations, such as here: https://github.com/NYULibraries/primo-explore-nyu-eshelf. That takes a bit more work though.

EDIT: I also see that https://github.com/alliance-pcsg/primo-explore-custom-actions/commit/07124a0e7633a8e31364ecabe89ddcce4534e00b was responsible for upgrading to 4, so if that's not necessary anymore we can revert that aspect of @jeremymcwilliams recent commit.

thatbudakguy commented 6 years ago

I'm fine with downgrading gulp to 3.9 for now; thanks for taking care of that. I think that commit was probably made to address some package vulnerabilities, but that shouldn't matter for a local dev setup.

Tests are passing on CI and locally for me, so I'm going to merge.

Thanks again for this big contribution!