-
Notifications
You must be signed in to change notification settings - Fork 506
fix: Touch interaction with search on mobile #798
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
base: master
Are you sure you want to change the base?
fix: Touch interaction with search on mobile #798
Conversation
* fix input width * move styles for conrtol back * chore: add section into docs about running app locally with https enabled * chore: add story to test out SearchBar pointer-events handling * nit: codestyle * chore: improve story to check for interactiveness criteria --------- Co-authored-by: Sergo <rogaldh@radsh.red>
|
@C0mberry 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 2800cc2 in 2 minutes and 5 seconds. Click for details.
- Reviewed
110lines of code in3files - Skipped
1files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. app/components/SearchBar.tsx:160
- Draft comment:
Add a comment explaining why the gridTemplateColumns style ('0 minmax(min-content, 1fr)') is applied to the input. This helps future maintainers understand it's a workaround for the react-select issue (github.com/JedWatson/react-select/issues/3857) and ensures proper mobile touch behavior. - 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 existing comment on line 163 already mentions the react-select issue #3857 as a workaround. The comment is positioned right after the input style block wheregridTemplateColumnswas added. While the existing comment specifically mentions it's for the placeholder style, it's reasonable to infer that the nearby input style changes are related to the same issue. The suggested comment would add clarity, but it's not strictly necessary since there's already a reference to the issue. The comment is asking for documentation/explanation rather than pointing out a code issue. According to the rules, comments should not be purely informative - they should require a code change. This is asking for a comment to be added, which is a minor documentation improvement but not a critical code issue. The existing comment might only apply to the placeholder style based on its position, and the newgridTemplateColumnsproperty might be solving a different aspect of the same issue. Adding an inline comment could genuinely help future maintainers understand why this specific CSS property is needed, especially since it's an unusual grid-related fix for an input element. While adding the comment could be helpful, the existing comment already provides a link to the issue where developers can find more context. The proximity of the existing comment (just one line after the input style block) makes it reasonably clear that these changes are related to the same workaround. This is more of a "nice to have" documentation improvement rather than a necessary code change. This comment is asking for additional documentation rather than identifying a code problem. While it could be helpful, there's already a comment referencing the react-select issue nearby. According to the rules, comments should identify clear code changes needed, not request additional documentation or explanations. This falls into the category of an informative/documentation request.
2. app/components/SearchBar.tsx:141
- Draft comment:
The PR description mentions resolving an issue regarding transaction history for deleted accounts, yet the changes here only address SearchBar styling. Ensure that either the description is updated or that additional changes addressing the transaction history are included. - 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.
3. app/components/stories/SearchBar.stories.tsx:39
- Draft comment:
The MobileContextMenuFix story effectively verifies that the input has a sufficient touch target width and proper pointer-events for interactivity. For further completeness, consider adding tests for keyboard navigation or accessibility of additional controls (like the clear indicator). - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. docs/development.md:15
- Draft comment:
The added mobile testing instructions with HTTPS are clear and helpful. As a minor note, consider mentioning that mobile devices (e.g., iOS Safari) might show warnings about self-signed certificates when running in development. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
Workflow ID: wflow_NpWjwPJSEtRPVwZJ
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Description
that fix allows you to interact with input on a mobile phone and as a result insert text
Type of change
Screenshots
video from mobile
Testing
Related Issues
HOO-216
Checklist