-
Notifications
You must be signed in to change notification settings - Fork 34
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
make dropdowns searchable #378
Conversation
I tried it on my phone and the dropdowns don't appear to work, works fine on the browser. Also, it'd be nice to have some cursor input indicator to reveal that this is an input box. |
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.
Fantastic job! Looks great to merge and functions beautifully.
Thanks @akgupta89 . I should note that I don't know if the menu placement will work with iOS since I read on some old posts that their keyboard doesn't fire the resize event. I tried testing it on heroku via browserstack (trial) but it doesn't seem to load at all, and I can't seem to get browserstack local to work either. I have no way of testing so it may have to be a separate issue for someone else if you want it fixed. Also using the back gesture/button to close the keyboard on mobile doesn't work because the input needs to be re-focused after the resize, which of course re-opens the keyboard. I don't know a solution to this at present. I am currently working on fixing the menu's exit transition if you want to wait for that. |
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.
Generally looks pretty nice. In addition to some feedback on the source code, I noticed some minor issues with the UI behavior listed below.
-
Agree that on mobile it would be nice to be able to close the keyboard without closing the select. Menu placement looks fine on iPhoneX. However, in portrait mode, you can only see 4 menu items. In landscape, you can only see one. Any attempt to scroll to see more fails, as the page scrolls back to show just the 4 or 1 again. I don't know to fix this, although I would say the automatic repositioning of the page seems kind of intrusive.
-
Noticed a weird issue on the iPhone where, after making a selection, if I tap on the select again, it keeps showing the "focus" animation (the dark line under the text input). Also, clicking on the menu items has no effect.
-
On Chrome on the Mac, when focusing the ReactSelect with the keyboard, it doesn't have background shading like the Material UI Select.
ReactSelect:
MUI Select:
-
On Chrome on the Mac, when opening the ReactSelect with the keyboard, the top menu item has keyboard focus. With MUI Select, the currently selected menu item is focused.
-
On Chrome on the Mac, when typing a search input, mouse clicks can be used to move the text cursor within the text. Click to the right of the text, though, closes the ReactSelect.
frontend/src/actions/index.js
Outdated
params: { query: query, variables: JSON.stringify(params) }, | ||
axios | ||
.get('/api/graphql', { | ||
params: { query, variables: JSON.stringify(params) }, |
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.
Doesn't look like anything significant was changed here? Can you revert changes in this file?
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.
sorry my bad habit of staging all files. I didn't actually make the change, I assume it was the Prettier code formatter that ran pre-commit
const menuPlacementTop = useRef(false); | ||
const [textFieldDOMRect, setTextFieldDOMRect] = useState({}); | ||
|
||
function handleReposition() { |
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.
There's a lot going on in this file. Can you add some comments to help explain what the issues and fixes are, like what happens if we don't have this resize/scroll handler?
} | ||
|
||
useEffect(() => { | ||
const menu = document.getElementById(`${selectProps.inputId}Menu`); |
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.
This logic is also pretty complicated. Can you add comments explaining what it does? Is it borrowed from somewhere? If so, cite the source here.
Terence makes a good point that this might look bad on small screens -- like a phone when the keyboard pops up. Indeed, it looked pretty bad for me: We agree we'd like to hide this typing input on phones. It's hard to judge what's a phone based on the screen size -- that screenshot is 2160 pixels tall (taller than my Macbook!). Judging based on the presence of a keyboard is hard and kinda flaky. Perhaps you could listen for a Or you could do it the quick and dirty way by sniffing the user agent for Android or iOS and use this StackOverflow answer to stop typing entirely. |
95919aa
to
4b41a8e
Compare
The focus issue was due to my novice mistake of declaring the components inside the select render so they were recreated each render.
Closing the soft keyboard should now work but after fixing the above issue makes pressing the select afterwards blur the input instead of bringing the keyboard back up. Is this still preferable over retaining focus and leaving the menu open on every click of the select? |
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.
See if you can fix the white-space-on-top bug I just mentioned.
sorry @hathix but I couldn't reproduce your issue. The only suspect I have is your device somehow scrolled after I locked page scrolling on menu open with I removed it from the heroku demo so see if you can reproduce it again. |
Yes, now I'm no longer getting that problem. |
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 merge this once you fix the merge conflicts.
I think I'm still seeing issues 3, 4, and 5. But the major problems seems to be gone. You can create new issues in github for those if you prefer. Also, just a reminder that we merged in some css-based map effected when you hover over a dropdown menu item. Hopefully not too hard to port over to your code, in the end it was based on some simple events. #416 |
No, it is the same. My menu placement only affects the dropdown menu. What you're seeing is react-select removing the placeholder/value when you type so the width is reverting to the min-width specified by the form control. The menu going offscreen as a result is a different story though, will have to update position onkeyup maybe. |
issue 4 seems to be a current react-select issue (JedWatson/react-select#3656), and I don't see a way to explicitly set their internal focus option to work around it aside from that I think all other issues mentioned above have been covered? |
Looks good, thanks for putting in so many fixes. Seems fine to wait for react-select to fix #4. |
Let's merge this once we fix the merge conflict. Anyone want to take that on? Doesn't look too awful. |
Wenyu, do you have the remaining merge conflicts fixed? |
Thanks @trxaphion and @zachzwy for fixing merge conflicts! |
Tip: if your PR is approved and just pending merge conflict fixes, you can go ahead and merge as soon as you've fixed the merge conflicts. |
Fixes #267
Proposed changes
as per the above issue the from stop and to stop selects have been replaced with react-select. I tried to emulate MUI's knick knacks as much as possible, although a few things are still unfinished. AFAIK there is:
exit transition for menu
limit input width
optionally make the menu react better to resizing window horizontally while menu is open
will have a go at them sometime soon but made the PR anyway in case there's any feedback
Link to demo, if any
https://open-transit-react-select.herokuapp.com/route/F/direction/F____O_E00
(p.s. yes I did post my username on slack)