-
Notifications
You must be signed in to change notification settings - Fork 43
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
[CT-3469] Refactor: add autocomplete to navbar, fix redirect #124
base: master
Are you sure you want to change the base?
Conversation
components/Autocomplete.tsx
Outdated
if (type==="topic") { | ||
router.push(`/packages/${encodeURIComponent(query?.fields?.package_name)}/versions/${encodeURIComponent(query?.fields?.version)}/topics/${encodeURIComponent(query?.fields?.name)}`); | ||
} else { | ||
router.push(`/search?q=${encodeURIComponent(query)}`); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also rewrite this as a switch case to handle all types 👍
Another thing for safety, you can push to the exact topics route if the params you use all exist, otherwise its a broken link so better to just send people to /search?q=${encodeURIComponent(query)}
case "package": | ||
router.push(`/search?q=${encodeURIComponent(query)}`); | ||
onSearch(); | ||
break | ||
case "search": | ||
router.push(`/search?q=${encodeURIComponent(query)}`); | ||
onSearch(); | ||
break | ||
default: | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just do:
case "package":
case "search":
router.push(`/search?q=${encodeURIComponent(query)}`);
onSearch();
break;
default:
break;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Well done on the changes brother ✊
There is one thing I realize the more I play around with the autocomplete, it's too weird that you can get suggestions that are completely unrelated to your search query. I think we should only suggest options that start with the same characters as what the user inputs. Then you just select the top 5 options that have the highest score (no need to set a threshold otherwise you will get the issue we had earlier where some inputs didn't have 5 options, just pick the top 5)
Here I think we should remove this, and just pick the 5 items with the highest score that start with what the user entered: rdocumentation-2.0/components/Autocomplete.tsx Lines 44 to 45 in bd0936d
So something similar to this:
|
Ticket
Making function suggestions redirect straight to topic page instead of global search results page.
Was going to do the same for packages suggestions, however there are some broken links so a risk that the suggestion will link to a "oops, page not found".
Changes
OnSearch function ensures that the searchbar clears and the autocomplete disappears once a search has been made. (This is has more of a ui effect on the navbar search as the autocomplete would cover search results otherwise).
I have also wrapped autosuggestion text to ensure there is no overflow.
I have changed the function redirect to point to specific topic pages and placed in a switch statement.
Pictures
Before (Desktop):
After (Desktop):
Before (Mobile):
After (Mobile):