platform-coop-toolkit / coop-map-directory-index

The Map/Directory/Index component of the Platform Cooperative Development Kit.
https://directory.platform.coop
BSD 3-Clause "New" or "Revised" License
1 stars 3 forks source link

feat: integrate Pinecone 1.0.0-beta.1 via NPM (resolves #54) #57

Closed greatislander closed 4 years ago

greatislander commented 4 years ago

Description

This PR adds Pinecone 1.0.0-beta.1 via NPM and consolidates all building/bundling/asset copying functionality using Laravel Mix. This means that Pinecone CSS and JavaScript are now included in the main CSS and JavaScript files (renamed to app.css and app.js respectively).

The PR also adjusts the card rendering to conform to the Pinecone standard, improving card link accessibility by making the cards focusable.

Steps to test

  1. Check out this branch.
  2. Run npm i to install updated dependencies.
  3. Test the scripts described in Additional information below and make sure that they work as expected.

Expected behavior: The application loads as expected with fonts, images, scripts and styles in place.

Additional information

The following NPM scripts are now in place:

Related issues

Resolves #54.

erictheise commented 4 years ago

Although it's common behavior in npm-land, the many warnings and errors flagged by npm run lint result in–

  62:5  warning  'point_counts' is assigned a value but never used     no-unused-vars
  63:5  warning  'totals' is defined but never used                    no-unused-vars

✖ 4958 problems (12 errors, 4946 warnings)
  0 errors and 4918 warnings potentially fixable with the `--fix` option.

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! coop-map-directory-index@0.1.0 lint: `eslint maps/static/maps/js/*.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the coop-map-directory-index@0.1.0 lint script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/erictheise/.npm/_logs/2020-05-22T17_33_03_453Z-debug.log

–which doesn't seem right. I'd expect it to report the warning and errors it found and then end silently as it's done its job.

May seem picky but it's a bad habit to learn to ignore error messages because it becomes easy to miss new ones that actually need to be addressed.

greatislander commented 4 years ago

@erictheise All of the "errors" were in all.js, which was copied from node_modules if I understand correctly. I've updated the PR to import it from node_modules and now the lint/test command now only produces warnings and exits gracefully. Re: this point:

May seem picky but it's a bad habit to learn to ignore error message because it becomes easy to miss new ones that actually need to be addressed.

CI and precommit hooks on all the other projects prevent commits/merges with lint warnings (let alone errors) so I don't tend to ignore these things.

erictheise commented 4 years ago

CI and precommit hooks on all the other projects prevent commits/merges with lint warnings (let alone errors) so I don't tend to ignore these things.

Apologies, didn't mean to cast aspersions @greatislander. Was speaking to personal experience in a local dev environment.

greatislander commented 4 years ago

@erictheise Added a line to the README as well— I'll expand further in subsequent work (probably add the scripts info from this PR to the README further down for convenient reference).