search

Giners / mui-places-autocomplete

Google Material Design (Material-UI) styled React component using Google Maps Places Autocomplete
MIT License
34 stars 26 forks source link

Fix (#13, #14): Render suggestions over (popover) components/elements #15

Closed Giners closed 6 years ago

Giners commented 6 years ago

This PR provides fixes for bugs:

The primary fix we are concerned with is #14. In providing a fix for #14 I dropped the use of react-autosuggest for downshift to provide dropdown/autosuggest functionality. I found it much easier to work with when trying to popover the suggestions container over some components/elements. By no means is this an endorsement that react-autosuggest isn't a suitable package/component to meet other peoples/projects needs. I'm sure I could have gotten things to work with it had I sunk lots more time into it (on top of a ton of time I already had) and/or wasn't so inexperienced. Anyways...

When reviewing please pay close attention to the changes made to `src/MUIPlacesAutocomplete.jsx' as that is where I need the most feedback/votes of confidence.

@oliviertassinari If you have time I would love for you to look things over so I can learn from your feedback/insights. I know you are busy and won't hold up the merge if you can provide feedback right now (or just don't desire to) but if you do get time to provide feedback in the future I can make changes retroactively.

oliviertassinari commented 6 years ago

@Giners Sorry, I will have no time to review your pull request. I'm happy to see you pushing the library forward :).

Giners commented 6 years ago

@oliviertassinari Thanks! :smile:

Totally understandable you don't have time to review the PR. I would like to continue to include you on the PRs (at least the larger/important ones) so I can learn from you. Is this alright? Of course if ever included on a PR there is never an obligation to review it, especially if you don't have time.

mattbailey commented 6 years ago

According to the snapshot, it looks like this change removes aria- properties. Might not be great for some users.

Giners commented 6 years ago

With the use of the 3rd party components <Downshift>/<Manager>/<Target>/<Popper> the serializations of our component has become massive and diffing them is now very time consuming. Instead of taking a snapshot of our component as a whole I now snapshot our rendered suggestions container (<MenuList>). That is why the snapshot is showing the loss of the aria properties.

Also there are non-deterministic properties associated with the <Downshift>/<Manager>/<Target>/<Popper> components. Snapshot testing won't work because of it.