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

Add multiselect preset #3080

Merged
merged 17 commits into from
Apr 30, 2016
Merged

Add multiselect preset #3080

merged 17 commits into from
Apr 30, 2016

Conversation

bhousel
Copy link
Member

@bhousel bhousel commented Apr 23, 2016

continued from #3055

kepta and others added 3 commits April 3, 2016 12:40
For multiselect fields,
field.key should have a ':' on the end of it - we use as the prefix, not the real key
field.keys should contain the array of real keys being modified

see https://github.com/openstreetmap/iD/blob/master/js/id/ui/preset.js#L17-L45

The address field preset works like this also..
It contains an array of key.fields for all the subfields that might be set.
For multiselect fields, we don't know ahead of time what all the subfields are,
so we append to the list in `change()` and rebuild list in `tags()`
@bhousel bhousel mentioned this pull request Apr 23, 2016
@bhousel
Copy link
Member Author

bhousel commented Apr 23, 2016

This is really good! Awesome work @kepta!

Currently I have these issues,

  • Combobox doesnt show dropdown if only one item is remaining.
  • On pressing enter editing exits.

First item is because d3.combobox defaults to minItems=2

  • we should probably change that to 1, because what if taginfo only fetches 1 thing

Second item: A few things with keypresses:

  • enter should accept the value but send focus to the input field
  • tab should do the same

I fixed a thing in 1a9c111:

  • Use field.keys so that undo and reset buttons work

Also, I noticed a lot of recycling values don't appear because, because:

  • combobox should probably fetch data on typing (like other combobox fields), not fetch data once on field initialization
  • and we should check that the taginfo queries are looking up enough values

Also I'll replace the input placeholder text with something translateable

  • "Type here" can just say "Add..."

@bhousel
Copy link
Member Author

bhousel commented Apr 26, 2016

and we should check that the taginfo queries are looking up enough values

^ related to this, I did some testing today and it looks like we need to adjust the popularity threshold values in the taginfo service code
We adjusted them before in #2748, but I think they're still too high.

In this case, we need to do a key lookup - but the key lookups have a higher threshold for popularity than the value lookups.

@bhousel
Copy link
Member Author

bhousel commented Apr 26, 2016

combobox should probably fetch data on typing (like other combobox fields), not fetch data once on field initialization

^ related to this, I realized today that the combo field does not fetch data on typing, but the raw tag editor value field does. This surprised me - I assumed they both had the bindTypeahead behavior from the raw tag editor..

So, I'm now thinking I want to add typeAhead to combo boxes too, and maybe merge together some of the code in combo.js and multiselect.js.

@kepta
Copy link
Collaborator

kepta commented Apr 26, 2016

@bhousel after looking at c592acb,

it was changed to 5000, so should we half it again?

@bhousel
Copy link
Member Author

bhousel commented Apr 26, 2016

it was changed to 5000, so should we half it again?

For normal keys it should probably be smaller than 5000. This usecase only applies in the raw tag editor when the user selects a key, so it's not as visible. The purpose is, ostensibly, to filter out garbage keys, but in practice some of the most garbage keys in OSM are also the most popular (tiger:*, ref:bag). This is also why we have code to sort keys with colons below keys without colons.

But.. for these multiselectable keys (recycling:*=yes) the threshold needs to be set very low, or changed to a popularity filter like value uses. It's a different usecase, so I want to think of a clean way to have taginfo.keys() serve both purposes - maybe pass another parameter (we already abuse parameter pretty bad).

@bhousel
Copy link
Member Author

bhousel commented Apr 30, 2016

This is coming along really well! I refactored combo.js and moved the multiselect code into there, because it turns out a lot of the code needed to be shared anyway.

Hoping to improve the tabbing behavior and merge soon!

@kepta
Copy link
Collaborator

kepta commented Apr 30, 2016

That's interesting, so now the combo type will be handling single or multiple values.

Nice job merging them into one, @bhousel :D

@bhousel
Copy link
Member Author

bhousel commented Apr 30, 2016

Looks awesome.. merging! :shipit:

multicombo1

@bhousel bhousel merged commit f071b93 into master Apr 30, 2016
@bhousel bhousel deleted the kepta-chips branch April 30, 2016 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants