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

fix: Position manager search and url generation not working #9219

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

memoyil
Copy link
Collaborator

@memoyil memoyil commented Mar 6, 2024


PR-Codex overview

The focus of this PR is to enhance filtering and searching functionalities in the PositionManagers feature.

Detailed summary

  • Improved filtering logic for URL query parameters
  • Implemented advanced search functionality using regular expressions

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

vercel bot commented Mar 6, 2024

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

Name Status Preview Comments Updated (UTC)
web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 22, 2024 8:14am
6 Skipped Deployments
Name Status Preview Comments Updated (UTC)
aptos-web ⬜️ Ignored (Inspect) Visit Preview Jul 22, 2024 8:14am
blog ⬜️ Ignored (Inspect) Visit Preview Jul 22, 2024 8:14am
bridge ⬜️ Ignored (Inspect) Visit Preview Jul 22, 2024 8:14am
games ⬜️ Ignored (Inspect) Visit Preview Jul 22, 2024 8:14am
gamification ⬜️ Ignored (Inspect) Visit Preview Jul 22, 2024 8:14am
uikit ⬜️ Ignored (Inspect) Visit Preview Jul 22, 2024 8:14am

Copy link

changeset-bot bot commented Mar 6, 2024

⚠️ No Changeset found

Latest commit: 9d1beb9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Mar 6, 2024

Typescript errors check

862 ts errors detected in all the codebase 😟.

Well done: no ts errors in files changed in this PR! 🎉

chefjackson/action-check-typescript

@memoyil memoyil force-pushed the feature/fix_position_manager_search_not_working branch from e3944ee to 9845968 Compare March 6, 2024 15:25
@pull-request-size pull-request-size bot added size/M and removed size/S labels Mar 6, 2024
router.asPath,
value
? {
[filterName]: encodeURIComponent(value.trim()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

updateQuery will do the encode so no need to encode again

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image Currently it is not encoding

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

Just tried on prod seems ok?

@@ -7,18 +7,26 @@ function filterHookFactory(filterName: string) {
const router = useRouter()
const filter = useMemo(() => {
const value = router.query[filterName]
if (Array.isArray(value)) {
return value[0]
if (value) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It will break if I empty the search field?

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 will not break, value will be undefined therefore it will return all positions

Copy link
Collaborator

Choose a reason for hiding this comment

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

but if the value changed from a valid string to an empty string, the query won't get updated?

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

was trying to delete the search and it's not working

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@memoyil memoyil force-pushed the feature/fix_position_manager_search_not_working branch from 9845968 to 76efb37 Compare March 7, 2024 09:30
@memoyil memoyil requested a review from chefjackson March 7, 2024 09:34
@memoyil memoyil force-pushed the feature/fix_position_manager_search_not_working branch from 76efb37 to 94206ad Compare March 8, 2024 11:11
@pull-request-size pull-request-size bot added size/S and removed size/M labels Mar 8, 2024
@memoyil
Copy link
Collaborator Author

memoyil commented Mar 8, 2024

@chefjackson You can review it again

const createSearchRegex = (wordText) => {
const words = wordText.split(/\s+/)
const escapedWords = words.map((word) => word.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'))
const pattern = escapedWords.map((word) => `(?=.*\\b${word}\\b)`).join('')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we support partial search? For example I'd prefer to be able to search bnb instead of typing wbnb

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

@memoyil memoyil force-pushed the feature/fix_position_manager_search_not_working branch from 7fbacce to 5441342 Compare March 11, 2024 10:36
@memoyil memoyil requested a review from chefjackson March 11, 2024 10:56
Comment on lines +12 to +24
return decodeURIComponent(value[0])
}
return decodeURIComponent(value)
}
return value
return undefined
}, [router.query])

const setFilter = useCallback(
(value?: string) => {
const trimmedValue = value?.trim()
router.push(
updateQuery(router.asPath, {
[filterName]: value,
[filterName]: trimmedValue && encodeURIComponent(trimmedValue),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to encode and decode again? updateQuery is using url-parse to parse and serialize the url, which uses querystringify to encode and decode the querystring

https://github.com/pancakeswap/pancake-frontend/blob/develop/packages/utils/clientRouter.ts#L1-L17

Copy link
Collaborator

Choose a reason for hiding this comment

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

May I ask what's the bad case here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants