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

feat: update query param on issues tab url based on search query #989

Merged
merged 8 commits into from
Nov 10, 2023

Conversation

j24m
Copy link
Contributor

@j24m j24m commented Nov 4, 2023

Developer name : @j24m

Backend Changes :

  • Yes
  • No

Frontend changes :

  • Yes
  • No

Is Under Feature Flag :

  • Yes
  • No

Database changes :

  • Yes
  • No

Breaking changes :

  • Yes
  • No

Tested on local :

  • Yes
  • No

Issue :

Description :

  • This pull request introduces the efficient representation of search query by updating it as query param on the issues tab address bar.

Code Changes :

  • utils/getQueryString.test.tsx : Efficiently handles the processing of search query in both the cases where there is a query string and where there is no query string i.e no value passed.
  • issues/index.tsx : If 'dev' is set to 'true', we update the URL by appending the 'q' parameter with a search prefix for a more meaningful URL structure. Additionally, we split and extract relevant search information from the URL query, so as to fetch issues correctly.
  • tests/Utils/getQueryString.test.ts : Provided test cases cover both empty and non-empty search queries, ensuring that the getQueryString function behaves as expected in all scenarios.

Anything you would like to inform the reviewer about:

Dev Tested :

  • Yes

Test Stats :

image
image
image

Images/video of the change :

20231107_182540.mp4

Follow-up Issues (if any) :

Copy link

vercel bot commented Nov 4, 2023

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

Name Status Preview Comments Updated (UTC)
status-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 10, 2023 10:13am

Copy link

@gauravsinhaweb gauravsinhaweb left a comment

Choose a reason for hiding this comment

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

  • The query should persist in the search bar after sharing.
  • The API should be search?q=dark%20mode or search?q=dark+mode (That's what Google use)

@j24m
Copy link
Contributor Author

j24m commented Nov 5, 2023

  • The query should persist in the search bar after sharing.
  • The API should be search?q=dark%20mode or search?q=dark+mode (That's what Google use)
  • Thankyou for pointing this out, I will persist the search query in search bar.
  • Coming to your second point I have constructed the query param as per discussion with @ankushdharkar, here is the screenshot for the same :
    image
    -So for issues it should be /issues?q=<some-key>%3A<dynamic-string>
  • Example : q=search%3Adark+mode

Copy link
Contributor

@joyguptaa joyguptaa left a comment

Choose a reason for hiding this comment

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

You haven't added a test case to verify whether the changes are actually reflected in the search URL or not when the input field is changed.

src/pages/issues/index.tsx Outdated Show resolved Hide resolved
src/utils/getQueryString.ts Outdated Show resolved Hide resolved
@Ajeyakrishna-k
Copy link
Contributor

Please add tests for your feature.

src/pages/issues/index.tsx Outdated Show resolved Hide resolved
src/pages/issues/index.tsx Outdated Show resolved Hide resolved
src/pages/issues/index.tsx Outdated Show resolved Hide resolved
src/pages/issues/index.tsx Outdated Show resolved Hide resolved
src/pages/issues/index.tsx Outdated Show resolved Hide resolved
src/pages/issues/index.tsx Outdated Show resolved Hide resolved
@gauravsinhaweb
Copy link

  • The query should persist in the search bar after sharing.
  • The API should be search?q=dark%20mode or search?q=dark+mode (That's what Google use)
  • Thankyou for pointing this out, I will persist the search query in search bar.
  • Coming to your second point I have constructed the query param as per discussion with @ankushdharkar, here is the screenshot for the same :
    image
    -So for issues it should be /issues?q=<some-key>%3A<dynamic-string>
  • Example : q=search%3Adark+mode

For this case, it looks good.

src/pages/issues/index.tsx Outdated Show resolved Hide resolved
src/pages/issues/index.tsx Outdated Show resolved Hide resolved
src/utils/getQueryStringFromUrl.ts Show resolved Hide resolved
src/utils/getQueryStringFromUrl.ts Show resolved Hide resolved
src/pages/issues/index.tsx Outdated Show resolved Hide resolved
src/pages/issues/index.tsx Outdated Show resolved Hide resolved
@j24m
Copy link
Contributor Author

j24m commented Nov 9, 2023

You haven't added a test case to verify whether the changes are actually reflected in the search URL or not when the input field is changed.

Done.

Copy link

@gauravsinhaweb gauravsinhaweb left a comment

Choose a reason for hiding this comment

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

The search should show closest suggestion of query
EX - If user search Dark mod missing e, the result should show for dark mode
currently it is showing the exact value to query

Screenshot 2023-11-10 112757

__tests__/Utils/getQueryStringFromUrl.test.ts Outdated Show resolved Hide resolved
src/pages/issues/index.tsx Outdated Show resolved Hide resolved
src/components/issues/SearchField.tsx Outdated Show resolved Hide resolved
src/pages/issues/index.tsx Outdated Show resolved Hide resolved
@j24m
Copy link
Contributor Author

j24m commented Nov 10, 2023

The search should show closest suggestion of query EX - If user search Dark mod missing e, the result should show for dark mode currently it is showing the exact value to query

Screenshot 2023-11-10 112757

Hello, regarding this comment of yours Gaurav : #989 (review)
The issue we are addressing in this task is only to update query params, the comment that you have put is out of scope for this issue.
Here is the issue link, acceptance criteria has been mentioned very clearly :
#971

@gauravsinhaweb
Copy link

The search should show closest suggestion of query EX - If user search Dark mod missing e, the result should show for dark mode currently it is showing the exact value to query

Screenshot 2023-11-10 112757

Hello, regarding this comment of yours Gaurav : #989 (review)
The issue we are addressing in this task is only to update query params, the comment that you have put is out of scope for this issue.
Here is the issue link, acceptance criteria has been mentioned very clearly :
#971

If the behaviour is same, after making your changes then it's fine.

Copy link
Contributor

@anishpawaskar anishpawaskar left a comment

Choose a reason for hiding this comment

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

LGTM

@prakashchoudhary07 prakashchoudhary07 merged commit 631ba78 into develop Nov 10, 2023
3 checks passed
@prakashchoudhary07 prakashchoudhary07 deleted the feat/update-query-params branch November 10, 2023 19:46
@j24m j24m mentioned this pull request Nov 10, 2023
13 tasks
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.

None yet

7 participants