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

BI-9: Make a simple search bar #3

Merged
merged 9 commits into from
Oct 11, 2023
Merged

BI-9: Make a simple search bar #3

merged 9 commits into from
Oct 11, 2023

Conversation

meenalnimje
Copy link
Contributor

[This is the code for simple search bar task. To test run the command npm run dev in web-app-client itself.

@@ -0,0 +1 @@
node_modules/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do it better over here. We don't need to keep the whole code of the repository. For now we only need the JSON file for the search bar, so let's limit only that file in this project.

To remove this folder, you'll need to untrack the whole folder. For that you'll need to run the command:

git rm -rf --cached web-scraping-script-main

The rm command will be used to remove files. Since it's a folder with a lot of files we'll need to run this operation recursively using the -r flag, the -f flag means forcefully, it's used here so that even if the files are modified, git won't be able to prevent the deletion. The --cached is used to untack the files. This will modify the .git folder such that it'll stop looking for changes in the provided file or directory.

@@ -0,0 +1,5357 @@
[
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need this file, so you can move this file to a new location. A suggestion from my side is to create a new folder called data which will act as the replacement of the script folder and then move this file over there.

Note: Try to do this operation using the mkdir and git mv command. In case you've trouble doing this, let me know. I can help you with that.

'use client'
import { useSearchParams } from 'next/navigation'
import jsonData from '../../../script/web-scraping-script-main/university_data'
import { useEffect, useState } from 'react'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can make the order of these imports better. Here's the priority:

  1. Library
  2. Custom Modules
  3. Local Files

So, the order of these three lines should be changed according to the priority.

export default function Home() {
const router = useSearchParams()
const collegeList = jsonData
const [collegeData, setCollegeData] = useState({})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contents of the JSON file won't change within the life of our client app. Due to this we don't need any state management for the JSON object.

So, you can do it like this:

  const id = router.get('id');
  const selectedCollege = jsonData.find((item) => item.id === id);

}, [id])
return (
<main className="flex min-h-screen flex-col items-center justify-center p-24">
<h1 className="text-5xl font-bold z-10">{collegeData.name}</h1>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the selectedCollege over here and please add chaining to prevent errors. You can do it like this:

selectedCollege?.name

src/app/page.js Show resolved Hide resolved
src/app/page.js Outdated
} else {
setOpen(true)
setSearchData(searchData)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if-else can be simplified.

setOpen(input.length !== 0);
setSearchData(filteredData);

src/app/page.js Outdated
<h1 className="text-5xl font-bold z-20">BacPac</h1>
<main className="flex min-h-screen flex-col items-center justify-start">
<h1 className="text-9xl font-bold z-20 mt-60">BacPac</h1>
<div className="searchBox mt-4 border-1 border-black w-4/12 h-12 rounded-2xl">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The seachBox shouldn't be camel case. It's more appropriate to use search-box to maintain consistency.

src/app/page.js Outdated
/>
{open && (
<div className="searchBox border-2 overflow-auto border-gray-300 w-full h-80 mt-4 rounded-lg p-3 bg-white">
{open &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to use open twice. The first one will assure whether the code block should execute or not.

@@ -13,7 +13,9 @@
"dependencies": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just leaving this comment over here, but it is unrelated to this code. There are some minor reasons due to which the build failed. We'll require a passing build to merge this code to the main branch.

@vercel
Copy link

vercel bot commented Sep 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
web-app-client ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 29, 2023 3:49pm

src/app/college/page.js Outdated Show resolved Hide resolved
src/app/page.js Outdated Show resolved Hide resolved

// More on how to set up stories at: https://storybook.js.org/docs/react/writing-stories/introduction#default-export
export default {
const btnStory = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While merging the code from the main branch, you can pick your changes. In case you need any help with the merge, let me know.

src/stories/Page.jsx Show resolved Hide resolved

import { Header } from './Header'
import './page.css'
>>>>>>> d2fbd219736b40e9ebe46e3fc3bb010704ec69ce
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may remove this line too.

@@ -35,15 +42,23 @@ export const Page = () => {
<ul>
<li>
Use a higher-level connected component. Storybook helps you compose
<<<<<<< HEAD
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this one.

such data from the of child component stories
=======
such data from the &quot;args&quot; of child component stories
>>>>>>> d2fbd219736b40e9ebe46e3fc3bb010704ec69ce
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 47-49 should be removed too.

</li>
<li>
Assemble data in the page component from your services. You can mock
these services out using Storybook.
</li>
</ul>
<p>
<<<<<<< HEAD
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this

@@ -10,7 +10,13 @@ const headerStory = {
layout: 'fullscreen',
},
}
<<<<<<< HEAD
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this line


export default headerStory
export const LoggedIn = {
>>>>>>> d2fbd219736b40e9ebe46e3fc3bb010704ec69ce
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

15-19 too.

@@ -21,8 +27,11 @@ const LoggedIn = {
const LoggedOut = {
args: {},
}
<<<<<<< HEAD
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this.

module.exports = {
headerStory,
LoggedIn,
LoggedOut,
}
=======
>>>>>>> d2fbd219736b40e9ebe46e3fc3bb010704ec69ce
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also lines 36 and 37

the
=======
the{' '}
>>>>>>> d2fbd219736b40e9ebe46e3fc3bb010704ec69ce
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also lines 83-85.

src/stories/Page.stories.js Show resolved Hide resolved
src/stories/Page.jsx Show resolved Hide resolved
src/stories/Page.jsx Show resolved Hide resolved
src/stories/Page.jsx Show resolved Hide resolved
src/stories/Page.jsx Show resolved Hide resolved
@bacpactech bacpactech changed the title search-bar feature added BI-9: Make a simple search bar Oct 11, 2023
@bacpactech bacpactech merged commit e8176f6 into main Oct 11, 2023
2 checks passed
@bacpactech bacpactech deleted the BI-9 branch October 11, 2023 05:53
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.

2 participants