Fix browser nav & remove the need to double-ENTER on the searchbar #298
Conversation
…avigation easier to use
…query to the searchbar
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@TyHil could you look over these questions? |
|
Idk what questions but I like all of these changes. What do you think of the commit I made to do the page title without an extra state? Also, intentional choice to capitalize UTD TRENDS? |
| @@ -181,6 +165,7 @@ const SearchBar = ({ | |||
| if (newValue.length) setErrorTooltip(false); //close the tooltip if there is at least 1 valid search term | |||
| setValue(newValue); | |||
| onChange_internal(newValue); | |||
There was a problem hiding this comment.
Also, I think we can remove this line since the new line below covers its functionality
| @@ -165,6 +165,7 @@ const SearchBar = ({ | |||
| if (newValue.length) setErrorTooltip(false); //close the tooltip if there is at least 1 valid search term | |||
| setValue(newValue); | |||
| onChange_internal(newValue); | |||
| onSelect_internal(newValue); // clicking enter to select a autocomplete suggestion triggers a new search (it also 'Enters' for the searchbar) | |||
There was a problem hiding this comment.
@TyHil Is this fine. newValue is a parameter for this function (it is an array with previous and the new SearchQueries). I am passing it in directly to onSelect_internal() instead of passing in value.
setValue sets value = newValue
I wanted to pass in value, but that does not get updated in time ig for onSelect_internal; newValue works fine and value eventually does get updated.
The other thing is there is a function handleKeyDown() that calls onSelect_internal when the enter key is pressed after adding all search terms (like I added cs1200 by clicking on it or SPACE or smtg, now I click enter).
Is it fine to have onSelect_internal to be called twice like this? Feels icky to mee
There was a problem hiding this comment.
newValue is perfect
I suppose if we can remove handleKeyDown and the code still works then that would be good
src/pages/dashboard/index.tsx
Outdated
| str = | ||
| str.lastIndexOf(', ') === str.length - 2 | ||
| ? str.substring(0, str.lastIndexOf(', ')) + ' - ' | ||
| : str; |
There was a problem hiding this comment.
I don't like this line. Is there a more efficient way?
It also has the fun property that if str is somehow length 1, the if condition is true -> it tries the substring
There was a problem hiding this comment.
Isn't str always empty until anything is added to it?
And running str.slice(0, -2) on any empty string causes no error and just returns an empty string.
So why not just str = str.slice(0, -2);
When is str ever length 1 tho? It's either nothing or it has a search term plus ", " added to it. Either way, I think the above solution works
…it can be dynamic with regard pageTitle
Nice!
yes, but what do you think? 11e2a2f the link preview says "Results - UTD Trends" and skips over pageTitle |
This reverts commit 673bbf4.
yeah i don't think it can be dynamic re: capitalization seems good, just needs to be changed in _app and _document to match will look at questions soon |
|
Browser Nav:
Searchbar ENTER: