fix: make SortBy control responsive on mobile#2142
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
DonKoko
left a comment
DonKoko
left a comment
There was a problem hiding this comment.
This PR doesn't exactly fix the existing bug, but rather completely replaces the sort by functionality on mobile.
A few things:
- This new design basically takes half the screen which I don't think is good.
Before:
After:
-
The way this is implemented is very strange. You are changing both
selectelements to "controlled", but not really as there is no state, they are controlled by the search params. If that is the approach they should just be kept as uncontrolled. Moreover this doesn't seem to be related to the resolution of the issue as the issue was defined as something happening because of the Popover focus on mobile, and that popover is removed so this seems necessary -
The code is quite messy and hard to understand. If it is really required to use a different component for mobile, it should be abstracted into its own file and everything should be kept clean and easy to understand and maintain
-
The tests have a lot of issues with them. They are not testing the actual code but rather testing the mocks. The actual important validations and behaviors of the component are not tested. IMO those tests are basically useless.
Replace implementation-detail testing with user-facing behavior tests: - Remove test for preventDefault call (implementation detail) - Remove assertions inside mock implementations - Add coverage for URL param validation logic - Add tests for orderDirection select (was missing) - Add tests for disabled state during navigation - Test preservation of other URL params Simplify mocking strategy: - Remove complex Radix mock that captured focus events - Use minimal Radix mock only to render content in JSDOM - All mocks have clear "why:" justification comments - No internal business logic is mocked Improve test organization: - Group tests by concern (initialization, interactions, disabled state, edge cases) - Clear, descriptive test names focused on observable behavior - Tests verify actual outcomes users care about All 14 tests pass and follow CLAUDE.md testing guidelines.
2 participants
Summary
Testing
https://chatgpt.com/codex/tasks/task_b_68f8e2ed58208326b78fdd7f1dea307c