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

#266 implement search functionality fe #271

Merged
merged 18 commits into from
Oct 21, 2023

Conversation

BelousSofiya
Copy link
Collaborator

@BelousSofiya BelousSofiya commented Oct 16, 2023

Frontend part of search functionality
image

onChange={(e) => setSearchTerm(e.target.value)}
/>
</div>
<button
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also would expect to run a search from the keyboard on the enter press

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you explain it, please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for feedback. Done.

Copy link
Collaborator

@popovycholeg popovycholeg Oct 20, 2023

Choose a reason for hiding this comment

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

I mean when I heat Enter button from the keyboard I would expect to run a search

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -1,4 +1,4 @@
.header-search-box {
/* .header-search-box {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Uncomment or remove

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for feedback. Done.

Comment on lines 27 to 37
axios
.get(`${servedAddress}/api/search/?name=${searchTerm}`)
.then((response) => {
setSearchResults(response.data);
setSearchPerformed(true);
setError(null); // Clear error on successful response
})
.catch((error) => {
console.error('Error fetching search results:', error);
setError(error.response ? error.response.data : 'An error occurred');
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can handle it using swr for the consistency

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for feedback. Done

import App from './App';
import reportWebVitals from './reportWebVitals';

const root = ReactDOM.createRoot(document.getElementById('root'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need to create a new react root for the SearchPage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for feedback. Done.

return (
<div>
{!error && (
<>
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for the fragment if you have only one child

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for feedback. Done.

@@ -0,0 +1,33 @@
import MainCompanies from './companies/Companies';
import './Text.css';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use module css

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for feedback. Done.

.new-companies {
display: flex;
align-items: center;
gap: 800px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it the correct value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It isn't a correct class. Deleted.


// those variables we would use for axios to get data from beckend
// get saved list code here
const usersSavedList = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to be a TODO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 14 to 18
// add company to saved list code here
// const addToSavedList = (profile_id) => {};

// del company from saved list code here
// const delFromSavedList = (profile_id) => {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Best practices writing comments I do not really think that it should be a comment. If this work is out of your ticket scope its better to create a new follow up ticket to complete it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for feedback. Done.

import { Link } from 'react-router-dom';
import wish_list_checklist from './img/wish_list_checklist.svg';
import wish_list_checklist_added from './img/wish_list_checklist_added.svg';
import './CompaniesCards.css';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use CSS modules

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for feedback. Done.

.map((category) => category.name)
.join(' ')}
</div>
<div className="product-card__name-text align_items_left">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a product card component? If no its better to move it to the separate component

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've renamed Companies.js to CompanyCard. Because it ctreates company card, not a list of companies. Is that smth you've meant?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it creates CompanyCard I think its ok.

Comment on lines 3 to 39
.App {
text-align: center;
}

.App-logo {
height: 40vmin;
pointer-events: none;
}

@media (prefers-reduced-motion: no-preference) {
.App-logo {
animation: App-logo-spin infinite 20s linear;
}
}

.main_block_outer {
padding-top: 10px;
}

.new-companies-main {
background-color: var(--new-companies-main);
}

.App-header {
background-color: var(--text-color);
min-height: 100vh;
display: flex;
flex-direction: column;
align-items: center;
justify-content: center;
font-size: calc(10px + 2vmin);
color: var(--white-color);
}

.App-link {
color: var(--link-color);
}
Copy link
Collaborator

@popovycholeg popovycholeg Oct 16, 2023

Choose a reason for hiding this comment

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

We probably want to include SearchPage to the main react root, some of the styles could be redundant than

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for feedback. Done.

</div>
</header>
);
function Header(props) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add prop types for props?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. It's not my task for now. But I'll create a new task

Copy link
Collaborator

@popovycholeg popovycholeg left a comment

Choose a reason for hiding this comment

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

With a couple of comments to resolve


const ITEMS_PER_PAGE = 6;

export function Search(props) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

prop types

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for feedback. Done

import CompanyCard from './companies/CompanyCard';
import styles from './Text.module.css';

const SearchResults = ({ results, displayedResults, isAuthorized }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

prop types

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for feedback. Done

import { useState, useEffect } from 'react';
import { useSWRConfig } from 'swr';
import useSWRMutation from 'swr/mutation';
// import useSWR from 'swr';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove if we do not use it here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

const [isSaved, setIsSaved] = useState(false);

async function sendRequest(url, { arg: data }) {
return fetch(url, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we use Axios across the app

Copy link
Collaborator Author

@BelousSofiya BelousSofiya Oct 20, 2023

Choose a reason for hiding this comment

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

Thank you for feedback. Done: fetch->axios

Authorization: `Token ${authToken}`,
},
body: JSON.stringify(data),
}).then();
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for the empty then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Comment on lines +49 to +51
mutate((key) => typeof key === 'string' && key.startsWith('/api/profiles/'), {
revalidate: true,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

<div className={styles['product-card__image-frame']}>
<img
className={styles['product-card__image']}
// src={companyData.banner_image}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

</div>
</div>
</div>
{/* {isAuthorized ? (isSaved ? filledStar : outlinedStar) : null} */}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be commented

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deleted

</div>
{/* {isAuthorized ? (isSaved ? filledStar : outlinedStar) : null} */}
{star}
{/* <div>{}</div> */}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deleted

Comment on lines 40 to 42
/* .search_result_error_search_value {
color: var(--main-style-signs-color);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deleted

@BelousSofiya BelousSofiya merged commit f95c32b into develop Oct 21, 2023
3 checks passed
@BelousSofiya BelousSofiya deleted the #266ImplementSearchFunctionalityFE branch October 27, 2023 13:52
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.

3 participants