Use typeahead on conversations · Pull Request #7129 · diaspora/diaspora

ghost · 2016-09-30T12:19:02Z

Typeahead results:

Conversation with one recipiant:

When recipient tag is hovered:

Flaburgan · 2016-09-30T12:36:38Z

Those are design propositions?

ghost · 2016-09-30T12:41:59Z

Yep. We can discuss the design of the tags.

Flaburgan · 2016-09-30T12:46:58Z

I'll be in favour of keeping the design as close as possible of the mention one. Unifying, you know :p

ghost · 2016-09-30T14:28:39Z

Ready for review.

svbergerem · 2016-09-30T22:35:19Z

I'd prefer a representation of the recipients that is similar to the representation in the dropdown: name and handle left aligned, the handle smaller than the name. The margins for the avatar should all be the same. (Currently the left margin is bigger.) I'd also like a smaller close icon that is in the right upper corner and uses an Entypo icon.

ghost · 2016-10-01T06:55:39Z

Yes, you're right, that's better:

But I prefer the close icon as it is, though:

ghost · 2016-10-03T08:17:39Z

BTW, the pills also show hovercard when hovered. I though it would be handy.

diaspora-code-review · 2016-11-01T13:21:17Z

-        post :create, @hash
-        expect(response).not_to be_success
-        expect(response.body).to eq(I18n.t("conversations.create.fail"))
+      context "with nil contact"  do


Unnecessary spacing detected.

ghost · 2016-11-01T16:10:36Z

Rebased and ready for another review.

svbergerem

Looks quite good so far. I reviewed everything except for the tests.

svbergerem · 2016-11-02T15:50:43Z

+    this.bindTypeaheadEvents();
+
+    this.getTagListElement().empty();
+    if (opts.prefills) {


Nitpick: I'd prefer opts.prefill.

svbergerem · 2016-11-02T15:51:19Z

-    });
-    $("#contact_ids").attr("aria-labelledby", "toLabel").focus();
+  addRecipient: function(person) {
+    var isRecipientAbsent = _(this.conversationRecipients).findIndex(_.matcher({handle: person.handle})) < 0;


We already use customSearch so this shouldn't be necessary.

I prefer to keep it because it is also used when prefilling.

How do you add users more than once when prefilling the list of recipients?

Our mentions view also uses the custom search and we don't need the extra check there.
prefillMention:

diaspora/app/assets/javascripts/app/views/publisher/mention_view.js

Line 128 in 2a927e6

prefillMention: function(persons) {

addPersonToMentions:

diaspora/app/assets/javascripts/app/views/publisher/mention_view.js

Line 45 in 2a927e6

addPersonToMentions: function(person) {

svbergerem · 2016-11-02T15:52:02Z

    }
  },

+  onRemoveRecipient: function(evt) {


Just removeRecipient since that's what this function does.

svbergerem · 2016-11-03T10:01:17Z

+
+  getTypeaheadElement: function() {
+    return this.$el.find("#contacts-search-input");
+  },


What's wrong with

this.typeaheadElement = this.$el.find("#contacts-search-input");

in initialize?

svbergerem · 2016-11-03T10:01:21Z

+
+  getContactsIdsListInput: function() {
+    return this.$el.find("#as-values-contact-ids");
+  },


this.contactsIdsListInput = this.$el.find("#as-values-contacts-ids");

svbergerem · 2016-11-04T23:08:11Z

@@ -0,0 +1,12 @@
+<div class="conversation-recipient-tag clearfix btn btn-primary" data-diaspora-handle="{{ handle }}">


This is not a button so I don't like using the btn class here. Changing the color on hover and showing the cursor as a pointer gives me the impression that you can click on it to change something. I'd rather just have the same background as the primary buttons.

svbergerem · 2016-11-04T23:14:28Z

    .form-group
-      %label#subject-label{for: "conversation-subject"}
-        = t(".subject")
+      %label#subject-label{for: "conversation-subject"}= t(".subject")


I think this was more readable before.

svbergerem · 2016-11-04T23:16:35Z

+      .recipients-tag-list.clearfix#recipients-tag-list
+      = text_field_tag "contact_autocomplete", nil, id: "contacts-search-input", class: "form-control"
+      - unless defined?(mobile) && mobile
+        = text_field_tag "person_ids", nil, id: "as-values-contact-ids", type: "hidden",


Why as-values-contact-ids? I thought as would be autoSuggest.

svbergerem · 2016-11-04T23:19:28Z

+      gon.push conversation_prefill: [current_user.contacts.find(params[:contact_id]).person.as_json]
    elsif params[:aspect_id]
-      @contact_ids = current_user.aspects.find(params[:aspect_id]).contacts.map{|c| c.id}.join(',')
+      gon.push conversation_prefill: current_user.aspects.find(params[:aspect_id]).contacts.map {|c| c.person.as_json }


Do we need these for the mobile view, too? Otherwise you could move this down into the else branch.

svbergerem · 2016-11-04T23:32:26Z

+    # This will have to be removed when mobile autocomplete is ported to Typeahead
+    recipients_param, column = [%i(contact_ids id), %i(person_ids person_id)].find {|param, _| params[param].present? }
+    if recipients_param
+      person_ids = current_user.contacts.where(column => params[recipients_param].split(",")).pluck(:person_id)


Shouldn't we use current_user.contacts.mutual... here?

Dunno. @jhass wrote this.

This is related to #7002. The UI already doesn't allow you to create a conversation with non-mutual contacts and doing the same in the controller would be the next logical step.

But that would be another change of behavior that would need some additional tests. So I'd say let's keep it like it is and change it to mutual contacts later.

ghost · 2016-11-13T10:55:02Z

Done.

because the desktop view doesn't use the name parameter anymore

* append html to spec.content() instead of body * don't append the unused conversations modal fixture * actually test that showModal has been called

* test that tag element are removed when clicking the tag's remove button

* append html to spec.content() instead of body * don't append the unused conversations modal fixture * actually test that showModal has been called

SuperTux88 · 2016-11-13T19:15:19Z

Merged.

Thanks @AugierLe42e and @svbergerem for the work!

Flaburgan · 2016-11-13T22:28:24Z

Awesome work guys :D

SansPseudoFix · 2016-11-14T12:45:49Z

Nice!
Fixes #5091, no?

svbergerem · 2016-11-14T12:59:56Z

@SansPseudoFix Yes, thank you!

ghost mentioned this pull request Oct 3, 2016

Port conversations backbone, use Typeahead pt. 2 and Bootstrap tag #6917

Closed

3 tasks

ghost mentioned this pull request Oct 29, 2016

Refactor conversation creation #7131

Closed

diaspora-code-review reviewed Nov 1, 2016

View reviewed changes

svbergerem requested changes Nov 4, 2016

View reviewed changes

Use typeahead on conversations

1e8500b

svbergerem added 6 commits November 13, 2016 15:10

Use !== instead of localeCompare when removing conversation recipients

76cf2b8

Move 'XSS via name parameter' to mobile conversation specs

9b7d559

because the desktop view doesn't use the name parameter anymore

Improve contacts page jasmine test

00626ac

* append html to spec.content() instead of body * don't append the unused conversations modal fixture * actually test that showModal has been called

Improve conversations form jasmine test

0483d0d

* test that tag element are removed when clicking the tag's remove button

Improve profile header jasmine test

8d27389

* append html to spec.content() instead of body * don't append the unused conversations modal fixture * actually test that showModal has been called

Remove unused conversations modal fixture

638cd60

svbergerem approved these changes Nov 13, 2016

View reviewed changes

SuperTux88 closed this in 117b17e Nov 13, 2016

SuperTux88 added this to the 0.6.2.0 milestone Nov 13, 2016

SuperTux88 mentioned this pull request Nov 13, 2016

More User infmation on conversations #7117

Closed

ghost deleted the port-conversations-typeahead branch November 13, 2016 20:19

svbergerem mentioned this pull request Nov 14, 2016

Can not pick one of several contacts with the same name when sending a message #5091

Closed

		@@ -0,0 +1,12 @@
		<div class="conversation-recipient-tag clearfix btn btn-primary" data-diaspora-handle="{{ handle }}">

Conversation

ghost commented Sep 30, 2016 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Flaburgan commented Sep 30, 2016

Uh oh!

ghost commented Sep 30, 2016

Uh oh!

Flaburgan commented Sep 30, 2016

Uh oh!

ghost commented Sep 30, 2016

Uh oh!

svbergerem commented Sep 30, 2016

Uh oh!

ghost commented Oct 1, 2016 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 1, 2016

Uh oh!

svbergerem left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 13, 2016

Uh oh!

SuperTux88 commented Nov 13, 2016

Uh oh!

Flaburgan commented Nov 13, 2016

Uh oh!

SansPseudoFix commented Nov 14, 2016

Uh oh!

svbergerem commented Nov 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ghost commented Sep 30, 2016 •

edited by ghost

Loading

ghost commented Oct 1, 2016 •

edited by ghost

Loading