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

feat(lookup-spa): add lookup-spa template #158

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mjg123
Copy link
Contributor

@mjg123 mjg123 commented Dec 2, 2020

Description

This template creates a front end as assets and a function which lets
folks use the Lookup API from their browser.

Checklist

  • I ran npm test locally and it passed without errors.
  • I acknowledge that all my contributions will be made under the project's license.

Notes

This template contains a compiled and minified JavaScript file in assets/lookup-spa-dist.js. The source for this is under assets-src and the config and npm run script are included in package.json, webpack.config.js and .babelrc.

If any of the source in assets-src is changed, then lookup-spa-dist.js should be regenerated as part of the same changeset. Do this by running npm install && npm run build-assets in the template root.

As I understand it, this would have to be a manual process at the moment.

@mjg123
Copy link
Contributor Author

mjg123 commented Dec 2, 2020

Tagging @dkundel and @philnash to discuss the implications of having a build-step as part of this template. I'm happy to take your advice here - as I see it there's a few options:

  1. Keep this as-is and rely on the manual process
  2. Rewrite this template to not need the build step
  3. Add some kind of hook to run npm run build-assets automatically

Option 1 seems risky. I'm not keen on option 2, personally. I think 3 adds flexibility for building larger apps as templates. But I don't know anything about where or how to add such a hook, so this is totally open for discussion.

@mjg123
Copy link
Contributor Author

mjg123 commented Dec 2, 2020

Side question: Was I supposed to include the .env file in my template? I didn't, because it's ignored in the top-level .gitignore, this suggests that maybe I should have https://github.com/twilio-labs/function-templates/blob/main/docs/CONTRIBUTING.md#adding-environment-variables

This template creates a front end as assets and a function which lets
folks use the Lookup API from their browser.
 - moved the `fetch` code to the top-level component
 - LookupInput and LookupResults are now stateless
 - `pn` renamed to `phoneNumber` for clarity
@mjg123
Copy link
Contributor Author

mjg123 commented Dec 8, 2020

Rebased to latest main and updated the code after @philnash reviewed it (Thanks!). I see that .env has been removed from .gitignore now so I have added it to this PR.

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.

1 participant