MLH-Fellowship / prep-project-4.1.1

MLH Prep Project for Pod 4.1.1
https://mlh-prep-4-1-1.netlify.app/
MIT License
6 stars 13 forks source link

Record previous searches #23

Closed ShiviPro closed 2 years ago

ShiviPro commented 2 years ago

Summary

This PR renovates the search functionality as well as stores the search history in Local Storage.

Details

This is PR 1 as we discussed in #12. Me and @Tanmay000009 have added the search functionality to work on both button press, and Enter(Return) keypress. Also we've stored the most recent 5 searches made by the user in the local storage, while checking if the current search is the same as the most recent search (in which case, we don't store it).

Related issues

netlify[bot] commented 2 years ago

✔️ Deploy Preview for mlh-prep-4-1-1 ready!

🔨 Explore the source changes: 75541ebb994e0372684e18a7889537ef97cdac35

🔍 Inspect the deploy log: https://app.netlify.com/sites/mlh-prep-4-1-1/deploys/616ff9e171c21d0008385ca5

😎 Browse the preview: https://deploy-preview-23--mlh-prep-4-1-1.netlify.app/

khattakdev commented 2 years ago

@ShiviPro @Tanmay000009 I see you folks are using localStorage but the data you save is never rendered on the screen.

Tanmay000009 commented 2 years ago

Hey @khattakdev, yes you are correct. So as we discussed in #12, we'll be making 2 PRs, so this is the first one. And 2nd one will be made after all of the major UI changes have been implemented as mentioned in other issues.

Also, I and @ShiviPro will take care of the conflict soon. Sorry for the delay.

ShiviPro commented 2 years ago

Hey @khattakdev, I've resolved the merge conflicts in App.js.

Also as @Tanmay000009 along with @quachtridat have come up in their discussion, mentioning dotenv import in the App, might limit team in terms of package usage; And that they should be free to use any package of their choice. Also, this would make the code more generic.

So I'm also removing the following import from App.js -

require("dotenv").config();

Also in respect to this same issue, please don't merge my first commit of chore: added npm pkg for env variables into main.

khattakdev commented 2 years ago

So I'm also removing the following import from App.js -

require("dotenv").config();

Also in respect to this same issue, please don't merge my first commit of chore: added npm pkg for env variables into main.

This is more of a STANDARD not Generic Code. There should be only one package use for such things. Else everyone will want to use the package of their own choice and we will end up using multiple packages for same purpose.

Moreover, your last request, you'll need to revert those changes, as merging this PR will also merge that commit.

ShiviPro commented 2 years ago

Hey @khattakdev, thanks for enlightening us with the difference between those 2 things there. Can you help us in suggesting what to do here ?

If we do it the standard way, there is no need to revert the changes done in chore: added npm pkg for env variables commit.

I just need to add the import again, while resolving current merge conflicts.

Shall we go with this, or is there something else we can go with here ?

khattakdev commented 2 years ago

@ShiviPro If there is any other package that's being used already, then you don't need this one. Go with the other one. Otherwise, you can keep it and use it.

ShiviPro commented 2 years ago

Hey @khattakdev, I've resolved the merge conflicts regarding this PR. I've used the dotenv as a standard package to access environment variables here. Kindly merge this, so that we can start working on the second PR of this as discussed in #12.

quachtridat commented 2 years ago

@ShiviPro you can choose to exclude part of changes on your local repo and commit + push only those changes that are necessary for the purpose of this pull request. dotenv will continue to exist in your local repo, and you can use that as you want. It can be done using interactive git add (and maybe some git stash to help save your code locally). Feel free to hit me up and I can tell more what I mean.