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

Autocomplete credit in search results panel #886

Merged
merged 7 commits into from
Jul 7, 2015

Conversation

kenoir
Copy link
Contributor

@kenoir kenoir commented Jul 2, 2015

Currently autocomplete doesn't function in the batch editing pane, and image viewer.

This PR uses the angular-xeditable typeahead element to maintain feature parity with the other xeditable elements in this panel.

@kenoir
Copy link
Contributor Author

kenoir commented Jul 2, 2015

Here's it working with Angular Material:

foo

I came across a couple of issues:

See: https://github.com/guardian/grid/compare/angular-md-autocomplete

@jamesgorrie
Copy link
Contributor

Did copy and paste work for the one we have?
Nice to have material design, but not if it takes ages.

@kenoir
Copy link
Contributor Author

kenoir commented Jul 2, 2015

I don't think it's a matter of taking ages, rather it's intended to be used in its entirety, so mixing and matching elements looks (and behaves) strangely.

There is also the matter of offering all the features that we want raised by the missing or broken functionality.

@kenoir kenoir changed the title Autocomplete Credit everywhere [WIP] Autocomplete Credit everywhere Jul 2, 2015
@kenoir
Copy link
Contributor Author

kenoir commented Jul 3, 2015

Working on search screen:
foo

@kenoir kenoir changed the title [WIP] Autocomplete Credit everywhere Autocomplete Credit everywhere Jul 3, 2015
@kenoir kenoir changed the title Autocomplete Credit everywhere Autocomplete credit in search results panel Jul 3, 2015
@@ -145,6 +146,7 @@ <h2 class="image-info__title" ng:if="!ctrl.hasMultipleValues(ctrl.rawMetadata.ti
credit
<span ng:class="{'image-info--multiple': ctrl.hasMultipleValues(ctrl.rawMetadata.credit)}"
editable-text="ctrl.metadata.credit"
e:typeahead="credit for credit in ctrl.credits($viewValue) | limitTo:8"
Copy link
Contributor

Choose a reason for hiding this comment

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

Great use of the plugin. When is it evaluated though? Just want to check it doesn't query the API on every Angular $digest cycle!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the way typeahead is implemented it hooks into the $parsers for the element the directive is attached to: https://github.com/angular-ui/bootstrap/blob/49e73a89f36b1c44d7547b8910bcb17e96135294/src/typeahead/typeahead.js#L195

Which I think means it's only modified when its' value changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. One way to test if you're not sure would be to trigger other digest cycles, e.g. select another image, etc, and see if it queries again. But it sounds like it wouldn't.

@kenoir
Copy link
Contributor Author

kenoir commented Jul 3, 2015

@theefer @akash1810 regarding implementing this on the image page. Could we discuss reusing the panel here?

I remember @paperboyo mentioning that the edit panel in Lightroom was the same in the search view / individual item view, might make more sense if both views have the same functionality, (and save us maintaining both views).

If we reckon it's worth doing, let's do it in another PR.

@theefer
Copy link
Contributor

theefer commented Jul 3, 2015

You might want to talk to @akash1810, they intrinsically have slightly different interaction patterns. +1 to reuse but not sure at which level it makes most sense.

@akash1810
Copy link
Member

Discussed IRL. 👍 to reuse!

Would also get a quick win in terms of displaying more metadata in selection mode; wether its editable or not is another question though - would you want to batch edit the location?

@theefer
Copy link
Contributor

theefer commented Jul 3, 2015

Worth shipping this first? 👍

@paperboyo
Copy link
Contributor

would you want to batch edit the location

Yeah - by dropping them all on a map 😜 !

Cool! A platonic +1 for reuse!

@kenoir
Copy link
Contributor Author

kenoir commented Jul 3, 2015

@theefer yes.

@theefer theefer force-pushed the autocomplete-credit-everywhere branch from 6b9b07c to 87d2c74 Compare July 4, 2015 23:55
@kenoir
Copy link
Contributor Author

kenoir commented Jul 7, 2015

Looks good in test, merging.

kenoir added a commit that referenced this pull request Jul 7, 2015
Autocomplete credit in search results panel
@kenoir kenoir merged commit ef49f25 into master Jul 7, 2015
@kenoir kenoir deleted the autocomplete-credit-everywhere branch July 7, 2015 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants