Feat/searchbar updates#524
Conversation
|
@nchigryay is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 24d48a2 in 2 minutes and 55 seconds
More details
- Looked at
637lines of code in8files - Skipped
2files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. app/scss/dashkit/_navbar.scss:366
- Draft comment:
The mobile navbar collapse animation uses 'max-height: auto' along with transitions. Consider using a fixed max height or a more robust CSS technique for smoother animations. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. app/components/ClusterStatusButton.tsx:21
- Draft comment:
Consider adding a default/fallback case to the switch on cluster status so that an unexpected status is handled explicitly. - Reason this comment was not posted:
Comment was on unchanged code.
3. app/components/Navbar.tsx:39
- Draft comment:
For clarity and maintainability, consider using a utility like 'classnames' for building dynamic class strings rather than manual interpolation. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. app/components/SearchBar.tsx:34
- Draft comment:
The constant RESET_VALUE is typed as 'any'. Consider using a more specific type (e.g. null or using the expected option type) for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment has a point - using 'any' is generally not ideal. However, looking at how RESET_VALUE is used, it's specifically meant to be an empty string to reset the select component. The 'as any' cast appears to be intentional to make TypeScript accept an empty string where it expects a SearchOption. This is a common pattern with react-select to clear the value.
The comment suggests using null as an alternative, but that might not work with react-select's API. The current implementation, while using 'any', might be the cleanest way to achieve the reset functionality.
While using a more specific type would be theoretically better, the current implementation is a pragmatic solution to work with react-select's API. The 'any' type here is intentional rather than careless.
The comment should be deleted. The use of 'any' here appears to be an intentional workaround for react-select's typing constraints, and changing it could make the code more complex without adding real value.
5. app/components/SearchBar.tsx:146
- Draft comment:
Instead of explicitly assigning 'DropdownIndicator' and 'IndicatorSeparator' to undefined in the components prop, consider omitting them entirely for cleaner code. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. app/layout.tsx:55
- Draft comment:
The layout renders the SearchBar inside the Navbar for large screens and re-renders it in a separate mobile container. Please ensure this duplication is intentional and well documented. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%
The comment is asking the PR author to ensure that the duplication is intentional and well documented. This violates the rule against asking the author to confirm their intention or ensure behavior is intended. However, it does point out a potential issue with duplication, which could be useful if rephrased to suggest confirming the intention without asking for documentation.
7. package.json:71
- Draft comment:
React-select has been updated to version ^5.10.1. Please verify that all type usages and component props are fully compatible with this new version. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify compatibility with a new version of a dependency, which violates the rule against asking the author to ensure compatibility or verify behavior. It does not provide a specific suggestion or point out a specific issue.
8. app/scss/dashkit/_navbar.scss:651
- Draft comment:
Typo: 'vertion' should be 'version' in the comment "Creates a small vertion of the vertical navbar". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. package.json:36
- Draft comment:
Potential typographical error: The dependency '@solflare-wallet/utl-sdk' on line 36 might be a typo. If this is unintentional, please consider correcting it to '@solflare-wallet/util-sdk'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_0zNNjdzZJ7vGVNab
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
This works great! I love that it autofocuses and the hotkeys work. |
ngundotra
left a comment
There was a problem hiding this comment.
Looks great! Thank you for this!
| import dynamic from 'next/dynamic'; | ||
| import { Rubik } from 'next/font/google'; | ||
| import { Metadata } from 'next/types'; | ||
| const SearchBar = dynamic(() => import('@components/SearchBar'), { |
There was a problem hiding this comment.
Despite the 'use-client,' the SearchBar is rendered on the server, which leads to warnings about attribute mismatches in select JedWatson/react-select#5859
The only suggestion was to avoid rendering it on the server, which is handled by the dynamic import.
Description
Type of change
Screenshots
Testing
1.
2.
Related Issues
Closes issue.
Checklist
Important
Updated search bar and navbar layout, added hotkeys, improved styling, and updated
react-selectdependency.SearchBartoNavbarfor large screens inlayout.tsx.SearchBarinSearchBar.tsx.react-selectto v5.10.1 and fixed related warnings inSearchBar.tsx.SearchBar.tsx.Navbarlayout to includeSearchBarandClusterStatusButtoninNavbar.tsxandlayout.tsx._navbar.scss.SearchBarcomponents in_solana.scssand_solana-dark-overrides.scss.Navbar.tsx.@mantine/hooksand updatedreact-selectinpackage.json.This description was created by
for 24d48a2. It will automatically update as commits are pushed.