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

Don't instantiate so many comboboxes #5568

Closed
bhousel opened this issue Dec 7, 2018 · 1 comment
Closed

Don't instantiate so many comboboxes #5568

bhousel opened this issue Dec 7, 2018 · 1 comment
Labels
bug A bug - let's fix this! field An issue with a field in the user interface performance Optimizing for speed and efficiency

Comments

@bhousel
Copy link
Member

bhousel commented Dec 7, 2018

A thing I found while working on #5558.

Every time we call d3_combobox() it makes a new one. This is ok to do when we want a new one, but really unnecessary when called on a d3 update selection (rerender).

input
.call(d3_combobox()
.container(context.container())
.fetcher(suggestNames(preset, allSuggestions))
.minItems(1)

This is a problem for 2 reasons:

  1. Performance (duh). We rebuild all the address combos every time the user types a letter.
  2. When we make a new one, we get all new state (Protip, I try to add _ to the front of variables that are state or act like state):

export function d3combobox() {
var dispatch = d3_dispatch('accept');
var _container = d3_select(document.body);
var _data = [];
var _suggestions = [];
var _minItems = 2;
var _caseSensitive = false;

And the reason that is a problem is because the _suggestions that have already been fetched go away.

So, if you have ever noticed that when using a combobox, you type in the field, you can no longer use the up/down arrows to navigate the combobox options - this is why. The suggestions don't come back until you do something that causes the fetcher to run again.

To fix this, I need to review all the places we use combo boxes in fields. I think that the main combobox field is ok, but the localized ones and the address ones and several others are not.

@bhousel bhousel added bug A bug - let's fix this! performance Optimizing for speed and efficiency field An issue with a field in the user interface labels Dec 7, 2018
@bhousel
Copy link
Member Author

bhousel commented Dec 11, 2018

Update: In e5dedef I refactored lib/d3.combobox.js -> ui/combobox.js
This means that there is nothing left in the /lib folder, which makes me happy.

A bit of background here. A long time ago D3.js had this concept of plugins, and some of the things that iD used were branched off from or intended to be released separately as plugins. This included the combo control, the keyboard handler, and the tiler. There are many others, but those were the ones used in iD.

D3 moved from v3 to v4 and nobody ever upgraded the plugins, and a community around D3 plugins really never took off. We still had a few of these things in the iD codebase that have diverged a bunch and were essentially iD-only modules stuck in the /lib folder, so I've moved them to the other parts of the code where they really belong.

  • lib/d3.combobox.js -> ui/combobox.js (uiCombobox)
  • lib/d3.geo.tile.js -> util/tiler.js (utilTiler)
  • lib/d3.keybinding.js -> util/keybinding.js (utilKeybinding)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug - let's fix this! field An issue with a field in the user interface performance Optimizing for speed and efficiency
Projects
None yet
Development

No branches or pull requests

1 participant