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

Suggestions #58

Open
5 tasks done
jackbridger opened this issue Dec 19, 2019 · 2 comments
Open
5 tasks done

Suggestions #58

jackbridger opened this issue Dec 19, 2019 · 2 comments

Comments

@jackbridger
Copy link

jackbridger commented Dec 19, 2019

I'm gonna say some things. I know you've had no time and been super ambitious by the way so I know some things you'll have identified :) And your project is awesome :)

  • Pin the footer to the bottom
  • Contrast the writing so it's more readable
  • Styling on the form if you get time (I blame myself for our form styling)
  • Clean and very helpful README with great instructions (i'd put instructions on separate lines). Also amazing that you have dbbuild - mention it in the README and also to add the .env file. Badges on readme are great though.
  • Sometimes your commit messages can be more specific (I'm the biggest offender of this)

Super obscure :
Firstly amazing that you used node_env for running tests.
Secondly amazing that you did error handling
When I tried to run your tests.
I initially set DATABASE_URL and not TEST_DATABASE_URL.
When I ran npm test,

I got the error 'Environment variable DATABASE_URL must be set' (even though it had been set).

let CURRENT_DB = process.env.DATABASE_URL;

if (process.env.NODE_ENV === "test") {
  CURRENT_DB = process.env.TEST_DATABASE_URL;
}

if (!CURRENT_DB)
  throw new Error("Environment variable DATABASE_URL must be set");

> Error: Environment variable DATABASE_URL must be set

A quick fix below:

let CURRENT_DB;
if (process.env.NODE_ENV === "test") {
  CURRENT_DB = process.env.TEST_DATABASE_URL;
  if (!CURRENT_DB) throw new Error(`Environment variable TEST_DATABASE_URL must be set`);
}
else {
  CURRENT_DB = process.env.DATABASE_URL;
  if (!CURRENT_DB) throw new Error(`Environment variable DATABASE_URL must be set`);
}

Helper Functions

I'd like to see this in a nicely named helper func (edit: realise you already had this as an issue - woo!)

  const name = req.url.split("=")[1];
  const formattedName = name[0].toUpperCase() + name.slice(1);
e.g. 
const formattedName = formatName(name);
@jackbridger
Copy link
Author

Amazing job AGIL!

@redahaq
Copy link
Collaborator

redahaq commented Dec 20, 2019

Thank you for your thoughtful and excellent suggestions @jackbridger! Please add Bailey as an honorary FAC18 Facster! 🐕 ❤️

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

No branches or pull requests

2 participants