search

chingu-voyages / v24-geckos-team-02

Books Plus | Voyage-24 | https://chingu.io/
GNU General Public License v3.0
1 stars 1 forks source link

Feature/readme #76

Closed Guitarhub786 closed 3 years ago

Guitarhub786 commented 3 years ago

@mokokom Thanks for the commit. I have refactored your contribution. Please call out if you want anything changed.

// FROM

// TO

snrelghgub commented 3 years ago

hello @Guitarhub786 I think you did an amazing job for our Readme.md; it is simple but to the point. I see that @mokokom also contributed to this - Awesome work guys! 👍

Guitarhub786 commented 3 years ago

@snrelghgub Thank you. I was happy with the features like the 'Table of contents'. However, I have simplified the file and taken on board @willnwhite suggestions that 'Table of contents' would suit a much larger project. He also suggested trimming down the 'Setup' as some of those instructions are build into Github.

snrelghgub commented 3 years ago

hello @Guitarhub786; I reviewed your Readme.md again & here are some of my few suggestions for improvement: -I suggest capitalizing the word API through your documentation since API is not a word but an acronym -I recommend the addition of the following checkboxes under the 'Features' section: UI/UX feedback & standards (since our app successfully provides the useful Output on loading & valid or invalid input & it was developed to meet at least a minimal UI/UX standard as researched & set by me + it was also reinforced during our reviewal process by everyone else on the team to improve user satisfaction)
Controlled Output (that would practically be a fancy but correct terminology to represent the fact that search results display with a function that renders a set of a card on a request-by-request basis to create an infinite scroll effect & as coded by @willnwhite ) Google Authentication (since our app nicely allows users to log in conveniently using Google credentials & as implemented by our brave teammate @ArunJose ) Responsive Menu or Mobile Menu (as implemented by myself & very nicely reviewed by you & @mokokom & to provide a better quality mobile user experience to include animations, sliding effect & where a user doesn't have to rely on one specific close button & can very conveniently click away to close it)

Feel free to refactor & merge my suggestions as you see fit with what is there already Cheers

snrelghgub commented 3 years ago

I also suggest: -the removal of "LIVE BETA LINK" as the objective of a beta link is only for development purposes to be used by developers or users we wish to give access to, to test & report issues during the final stages of development; One "LIVE LINK: " of our production deployment for demonstration purposes shall suffice. -a screenshot of our app, preferably a GIF animation but an image alone showcasing the project in the best way you can visually is also definitely a must have Cheers

ghost commented 3 years ago

Sorry about all these "starting review" comments. I don't understand the feature!

snrelghgub commented 3 years ago

"UI/UX feedback & standards" isn't a feature as such but it does deserve being talked about. "Controlled Output" would be covered by "Infinite Scroll"

suggestions often lead to better decisions & I agree 100% with your inputs

snrelghgub commented 3 years ago

@Guitarhub786 As a rule of thumb, features need be promoted before providing installation instructions, please review the order of the information presented in our Readme & also suggestions for improvements you have ignored & not implemented yet in my previous comments.

ArunJose commented 3 years ago
  1. In features, "landing page" can be changed to "Search books by name or author". This better explains the key feature of our app
  2. In features, please add "Autocomplete based on search history"
  3. .env file should be at the root of react project not inside src folder
  4. Where to get API key? https://developers.google.com/books/docs/v1/using#APIKey. We use both kind of credentials mentioned in this section.
  5. I think it would be better to remove "Bootstraped with CRA" and "React" from Dependencies as React is a dependency of CRA. It's fairly obvious that we are using React with CRA, and React.js is already mentioned in Tech Stack
  6. Setup section can be improved showing steps for installation in the required order. For example, please see https://github.com/othneildrew/Best-README-Template#getting-started
  7. There are numbered lists, checkboxes and bullet lists in the document. I think it would look nicer if all of those are converted into bulleted lists.
  8. The current link to medium post(for obtaining Google Client ID) although informational, explains usage in context of an unrelated Node application. That link was only meant as a reference. Under "Obtain the OAuth client ID and client secret" in that page it shows how to get Google Client ID and Client secret. Please not we only need Client ID for our app. The following is a better link to include instead: https://developers.google.com/identity/protocols/oauth2/javascript-implicit-flow#creatingcred
  9. The comment inside .env file // Add your own API keys here isn't necessary. If it's needed please double check on .env comment syntax. In most places .env comments is shown to start with a #. Please make sure this works locally before adding to documentation.
ghost commented 3 years ago

Team developers are in alphabetical order.

ghost commented 3 years ago

I'm going to try to exclude the non-README files from this PR.

ghost commented 3 years ago

I'm going to try to exclude the non-README files from this PR.

I thought Case 3 on https://ffeathers.wordpress.com/2020/03/21/removing-an-updated-file-from-a-pr-on-github/ would work but it didn't. I have force-pushed the branch back one commit for now.

snrelghgub commented 3 years ago

Team developers are in alphabetical order.

That's a great idea, Sorry I didn't notice your comment until now; it is worth mentioning (in alphabetical order) above developer list if it is the case 👍

ghost commented 3 years ago

The GIF is too slow so I'll speed it up.

ArunJose commented 3 years ago

@willnwhite The current version of setup section gives out the impression that API key and client ID are optional for our app. I've rewritten the section with the changes I would like.

Setting up for development

You'll need npm to install the project's dependencies.

  1. Create an API key for accessing Google Books API and OAuth Client ID for using Google login. When making the client ID, set the origin URI to match the app's URI (http://localhost:3000 by default).

  2. Clone this repository.

  3. Now run: 

    npm install

  4. Create a .env file in the project's root directory, which has this format.

    REACT_APP_GOOGLE_BOOKS_API_KEY=MY_BOOKS_API_KEY
REACT_APP_GOOGLE_CLIENT_ID=MY_CLIENT_ID

    Replace MY_BOOKS_API_KEY with your API key, and MY_CLIENT_ID with your OAuth Client ID.

  5. Now run: 

    npm start

    This should start the project in a new browser window.

ghost commented 3 years ago

@ArunJose I figured that the only reason someone would run the app on their own computer would be to develop it, as the app is already available online for users. The API key and client ID aren't necessary if you want to eg develop the styling (the app still starts up without them).

ArunJose commented 3 years ago

@willnwhite You're right the app will start without API key and Client ID. But we are writing documentation for a someone to run the current version of BooksPlus app locally with its full features mentioned in the features list.

If API key and client ID are to be skipped, it should be mentioned what functionalities one would lose as someone cloning and running this project will not have prior idea why some of the features aren't working.

ghost commented 3 years ago

But we are writing documentation for a someone to run the current version of BooksPlus app locally with its full features mentioned in the features list.

This part of the readme is for developers only, not for someone who wants to have the full features of the app out of the box.

ArunJose commented 3 years ago

@willnwhite I think even for developers it's expected behaviour that app would function with its full features after going through offical installation instructions.

I believe these instructions should be meant for anyone who would like to run a copy of this app locally, irrespective of their intention to develop this further, or their level of Javascript knowledge.

One scenario is that the deployment link is broken or API key/Client ID is invalid/blocked, and someone would just like to run the app locally to see it working.

Also I think we shouldn't discard possibility of someone who just want test this out before making an independent deployment without any intention to modify the app.

It's fine if you want to mention that the app will run without setting environment variables set but in that case it should be mentioned what features will not work as a result. Otherwise after installation one will have to figure out why the login button has become unresponsive and mistake it as a bug until they go through code to understand what's happening under the hood.

ghost commented 3 years ago

I asked @jdmedlock to review and he agrees with @ArunJose that the API key and client ID shouldn't be optional, so I've used @ArunJose's re-write from comment https://github.com/chingu-voyages/v24-geckos-team-02/pull/76#issuecomment-731732145. Ready for review!

ghost commented 3 years ago

Nice catch @ArunJose!