Update vanilla handlebars code to what worked for me

cloudfour / core-hbs-helpers

Handlebars helpers that we usually need

MIT License

11 stars 2 forks source link

Update vanilla handlebars code to what worked for me #49

Closed Paul-Hebert closed 5 years ago

Paul-Hebert commented 5 years ago

The code in the readme did not work for me without a couple changes. This PR updates the readme with those changes:

Fix require name
Iterate over helpers as an object, not an array

spaceninja commented 5 years ago

The code looks right, and I trust that Paul tested and it's working as intended, but I'll defer to those with more knowledge of the repo.

@tylersticka unfortunately, looking at the commit history, I think that's you.

gerardo-rodriguez commented 5 years ago

Out of curiosity, would things have been different if this would've been installed?

https://www.npmjs.com/package/@cloudfour/hbs-helpers

It's been a while (over 1.5 years) since I've installed/used our helpers due to the projects I've been on. I suppose this also makes me wonder if the documentation install step needs to be updated as well to install from NPM? 🤔

Paul-Hebert commented 5 years ago

@gerardo-rodriguez , that's a good question. I'm not sure. I followed the installation steps from the README and ran this command: npm install --save-dev cloudfour/core-hbs-helpers.git

That resulted in this being added to my package.json: "@cloudfour/hbs-helpers": "github:cloudfour/core-hbs-helpers",

Should I have installed that alternate package?

gerardo-rodriguez commented 5 years ago

@gerardo-rodriguez , that's a good question. I'm not sure. I followed the installation steps from the README and ran this command: npm install --save-dev cloudfour/core-hbs-helpers.git

That resulted in this being added to my package.json: "@cloudfour/hbs-helpers": "github:cloudfour/core-hbs-helpers",

Should I have installed that alternate package?

Good question, @Paul-Hebert. Technically, either is fine. There was an effort some time ago to get our shared packages published on NPM. I would imagine it seems we should default to installing from NPM.

Do you agree @cloudfour/dev?

Paul-Hebert commented 5 years ago

That makes sense to me. If that's the case I wonder if it's the install step that needs to be updated instead of the code examples.

gerardo-rodriguez commented 5 years ago

That makes sense to me. If that's the case I wonder if it's the install step that needs to be updated instead of the code examples.

@Paul-Hebert That is the same question I have. :)

Would you be able to test it out? 😬

gerardo-rodriguez commented 5 years ago

Also, FWIW, the last time I was on a project that used these HBS helpers (again, a long time ago in web years) it was using Gulp so the Vanilla example may have been incorrect and we never caught that. 🙈

Paul-Hebert commented 5 years ago

@gerardo-rodriguez yeah, sure. I can test the npm package, and see what's what with the vanilla instructions.

Paul-Hebert commented 5 years ago

@gerardo-rodriguez , I ran into the same problems when using the npm package.

The package name in README was incorrect
The vanilla JS example treated helpers as an array instead of an object.

gerardo-rodriguez commented 5 years ago

@gerardo-rodriguez , I ran into the same problems when using the npm package.

The package name in README was incorrect

The vanilla JS example treated helpers as an array instead of an object.

That makes sense, thanks for testing @Paul-Hebert! 👍

Paul-Hebert commented 5 years ago

Sure thing!