Skip to content
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

[docs][Autocomplete] Fix GoogleMaps demo #35545

Merged
merged 12 commits into from Dec 22, 2022

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Dec 20, 2022

Closes #35391

Problem:

Before:
(1) Go to https://mui.com/material-ui/react-autocomplete/#google-maps-place
(2) Enter "Darse de Baudour"
(3) Demo crashes

After:
(1) Go to https://deploy-preview-35545--material-ui.netlify.app/material-ui/react-autocomplete/#google-maps-place
(2) Enter "Darse de Baudour"
(3) Demo doesn't crash

@hbjORbj hbjORbj self-assigned this Dec 20, 2022
@mui-bot
Copy link

mui-bot commented Dec 20, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-35545--material-ui.netlify.app/

No bundle size changes

Generated by 🚫 dangerJS against acc32bd

@hbjORbj hbjORbj force-pushed the fix/autocomplete-demo branch 2 times, most recently from b80b128 to fce90bb Compare December 20, 2022 16:19
@hbjORbj hbjORbj added bug 🐛 Something doesn't work docs Improvements or additions to the documentation component: autocomplete This is the name of the generic UI component, not the React module! labels Dec 20, 2022
@hbjORbj hbjORbj changed the title Draft: [docs][Autocomplete] Fix demo Draft: [docs][Autocomplete] Fix GoogleMaps demo Dec 20, 2022
@hbjORbj hbjORbj changed the title Draft: [docs][Autocomplete] Fix GoogleMaps demo [docs][Autocomplete] Fix GoogleMaps demo Dec 20, 2022
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking at this bug, there are quite a few open-source projects just to solve this problem, so people will appreciate a quality demo 👍. A few more ideas to improve the Google Maps place demo:

  1. There is an overflow issue, e.g. when searching for "zzzz"

Screenshot 2022-12-21 at 01 16 21

  1. Should we customize the noOptionsText prop? Maybe like "No places" would make more sense than "No options"?
  2. The warning "Before you can start using the Google Maps JavaScript API and Places API, you must sign up and create a billing account." feels unclear. It doesn't explain why. I think that it should mention that it's to get an API key.
  3. We frequently hit a rate limit error. I have configured some of it in our Google Cloud dashboard so we stay inside the free tier, but I don't understand why we get them. Maybe we could set the throttle at 400ms rather than 200ms?

Screenshot 2022-12-21 at 01 37 19

  1. Considering how we solve the problem of out-of-order requests, using throttle makes no sense. We effectively have a debounce, so we might as well use a debounce to make fewer network requests.

docs/data/material/components/autocomplete/GoogleMaps.tsx Outdated Show resolved Hide resolved
@hbjORbj
Copy link
Member Author

hbjORbj commented Dec 21, 2022

  1. There is an overflow issue, e.g. when searching for "zzzz"

Fixed.

  1. Should we customize the noOptionsText prop? Maybe like "No places" would make more sense than "No options"?

Agree. I will go with "No locations".

  1. The warning "Before you can start using the Google Maps JavaScript API and Places API, you must sign up and create a billing account." feels unclear. It doesn't explain why. I think that it should mention that it's to get an API key.

Yes, "Before you can start using the Google Maps JavaScript API and Places API, you need to get your own API key." sounds better.

@hbjORbj
Copy link
Member Author

hbjORbj commented Dec 21, 2022

  1. We frequently hit a rate limit error. I have configured some of it in our Google Cloud dashboard so we stay inside the free tier, but I don't understand why we get them. Maybe we could set the throttle at 400ms rather than 200ms?
  2. Considering how we solve the problem of out-of-order requests, using throttle makes no sense. We effectively have a debounce, so we might as well use a debounce to make fewer network requests.

Replaced throttle with debounce from '@mui/material/utils' and changed 200ms to 400ms in this commit

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed a few more changes that were not directly related to the initial problem but seemed to be a good opportunity to modernize the example, to use the features that were not available when the demo was first created.

I can't find anything else to improve 👍 , this is a much better demo 😄

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 21, 2022

Off-topic. I have just realized that the undo Ctrl+Z and redo shortcuts do nothing on our autocomplete. It's a bit frustrating. It works in Gmail:

Screenshot 2022-12-21 at 21 28 00

I can't find any UI component that supports this (select2, react-select, downshift, react-spectrum, antd, they don't), so maybe an opportunity to get an edge on the UX side. Or maybe it's overkill 🤷‍♂️

@hbjORbj
Copy link
Member Author

hbjORbj commented Dec 21, 2022

I can't find any UI component that supports this (select2, react-select, downshift, react-spectrum, antd, they don't), so maybe an opportunity to get an edge on the UX side. Or maybe it's overkill 🤷‍♂️

I don't really think it's an overkill ;) It's quite important for UX that undo/redo works.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No way this really happened (I wanted to test the "No locations" scenario) :D

image

The updates look good 👍

@hbjORbj hbjORbj merged commit bcb6d57 into mui:master Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs][Autocomplete] Demo GoogleMaps crashes
4 participants