Skip to content

[docs][Autocomplete] Fix GoogleMaps demo#35545

Merged
hbjORbj merged 12 commits intomui:masterfrom
hbjORbj:fix/autocomplete-demo
Dec 22, 2022
Merged

[docs][Autocomplete] Fix GoogleMaps demo#35545
hbjORbj merged 12 commits intomui:masterfrom
hbjORbj:fix/autocomplete-demo

Conversation

@hbjORbj
Copy link
Contributor

@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 type: bug It doesn't behave as expected. docs Improvements or additions to the documentation. scope: autocomplete Changes related to the autocomplete. This includes ComboBox. 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 force-pushed the fix/autocomplete-demo branch from fce90bb to 09842f8 Compare December 20, 2022 16:37
@hbjORbj hbjORbj force-pushed the fix/autocomplete-demo branch from 61ffec2 to 2f15e33 Compare December 20, 2022 17:24
@hbjORbj hbjORbj requested a review from mnajdova December 20, 2022 17:40
@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.

@hbjORbj
Copy link
Contributor 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
Contributor 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
Contributor 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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Improvements or additions to the documentation. scope: autocomplete Changes related to the autocomplete. This includes ComboBox. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[docs][Autocomplete] Demo GoogleMaps crashes

4 participants