Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

[bug]:corrected the search functionality #9788

Closed
wants to merge 8 commits into from

Conversation

jatingodnani
Copy link

closes #9738

  • [*] My code follows the code style of this project.

  • [* ] My change requires changes to the documentation.

  • [ *] I have updated the documentation accordingly.

  • [* ] All new and existing tests passed.

  • [ *] This PR does not contain plagiarized content.

  • [ *] The title of my pull request is a short description of the requested changes.

Added loader indicator so that if any search, intially at the query time it shows the recently updated user but i have done some changes or added to the code so it showing loading indicatior, not the recently updated users.

@github-actions github-actions bot added the issue linked Pull Request has issue linked label Nov 14, 2023
pages/search.js Outdated Show resolved Hide resolved
Copy link
Member

@sital002 sital002 left a comment

Choose a reason for hiding this comment

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

The package-lock.json file shoulnot be changed unless you are installing a new package please revert those changes and one more thing please remove all the console.log statement that was used for debugging.

pages/search.js Outdated Show resolved Hide resolved
Copy link
Member

@eddiejaoude eddiejaoude left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution 👍

I have not tested it yet, but please check your Prettier settings, so it is easier to review the "actual" lines changed

@jatingodnani
Copy link
Author

@eddiejaoude review my code

pages/search.js Outdated Show resolved Hide resolved
pages/search.js Outdated Show resolved Hide resolved
pages/search.js Outdated Show resolved Hide resolved
@eddiejaoude
Copy link
Member

Seems like the search tests are failing

@jatingodnani
Copy link
Author

Why this is failing,in my local host it is working fine
Is there any reason

@eddiejaoude
Copy link
Member

Why this is failing,in my local host it is working fine Is there any reason

the automated tests pass for you locally?

@jatingodnani
Copy link
Author

I have change some ui,which is working fine in my local
Can you pls test my code,then let me know why this is failing

@eddiejaoude
Copy link
Member

Why this is failing,in my local host it is working fine

can you please share the results of running the tests locally

I have change some ui,which is working fine in my local Can you pls test my code,then let me know why this is failing

you can see the failing tests in the GitHub Action

@jatingodnani
Copy link
Author

@eddiejaoude help me to find why 5th case is failing

@badrivlog
Copy link
Contributor

@jatingodnani please resolve the merge conflicts

@eddiejaoude
Copy link
Member

eddiejaoude commented Dec 1, 2023

It seems like the test checks are expected different results before as to now

1) [chromium]  search.spec.js:22:5  Search works correctly ─────────────────────────────────────

    Error: Timed out 5000ms waiting for expect(locator).toHaveCount(expected)

    Locator: locator('main li')
    Expected: 1
    Received: 0

Please run the tests and check your changes.

https://github.com/EddieHubCommunity/BioDrop/actions/runs/6913193428/job/18823909700?pr=9788

note: I don't think we need a loading state, when we upgrade to the latest nextjs, this will come in as part of that

@adityaraute
Copy link
Member

@jatingodnani were you able to resolve the conflicts?

@jatingodnani
Copy link
Author

jatingodnani commented Dec 27, 2023

Yes,i solved this problem ,but i am not understand why my test case is failing
@eddiejaoude can you pls tell me is this migrated to nextjs?

Comment on lines +138 to +140
return () => {
clearTimeout(timer);
};
Copy link
Member

Choose a reason for hiding this comment

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

Does the coding style need to be changed here?

<div className="flex items-center justify-center mt-8 w-[97%]">
<div
class=" animate-spin rounded-full border-opacity-25 h-10 w-10 border-4
border-t-blue-500 "
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
border-t-blue-500 "
border-t-blue-500"

</ul>

{loading && (
<div className="flex items-center justify-center mt-8 w-[97%]">
Copy link
Member

Choose a reason for hiding this comment

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

97% seems a bit strange?

@eddiejaoude
Copy link
Member

  1. please resolve conflicts
  2. test is failing because the expect results has changed

From the GitHub Action, you can see...

Expected: 9
Received: 27

@SaraJaoude
Copy link
Member

Given the length of time that this PR has been open and maintainer comments have not been addressed, I will close this to give someone else in the community an opportunity to work on it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue linked Pull Request has issue linked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Search page rolls back to initial state whenever a new search filter is added/removed.
6 participants