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

4055 add contacts mobile #5594

Merged
merged 1 commit into from Feb 15, 2015

Conversation

Projects
None yet
8 participants
@Faldrian
Copy link
Contributor

Faldrian commented Jan 26, 2015

Added a "add contact" dropdown in mobile view.
A user can add and remove people from existing aspects in mobile view.
I use the first option in the select field to be a cover reflecting the current state (like the desktop-dropdown does). Since this option is marked disabled and shown in the popup, the user has some overview of the current status when adding a person to aspects.

Why select in single-select-mode and not "multiple"?
Adding people to aspects is a "choose multiple" task, similar to selecting aspects in the publisher. The multiple-select HTML-element would be perfect for that. Instead I did some JS-magic with the single-mode-select because mobile browsers are much more predictable how they display the select (firefox mobile was not very usable with multiple select).

The checks and dots are not well aligned!
Yeah... since I did not find a way to style the selection popup. This is the best I could do with utf8 to let it look okay... If someone has a better idea, please comment.

This will solve #4055.

(I have to write tests now...)

Screenshots (Firefox mobile on Android):
screenshot_2015-01-27-10-35-16

profile2

Google Chrome on Android:
screenshot_2015-01-27-10-38-05

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jan 26, 2015

You're the man!

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jan 26, 2015

Would you mind indicating on bountysource that you are working on this? You will help us know how this tool works :p

@SansPseudoFix

This comment has been minimized.

Copy link
Contributor

SansPseudoFix commented Jan 26, 2015

Yeah, big thanks for that ! Many users want that feature !

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 26, 2015

@SansPseudoFix I know, that's why it was now top of my list and I got time to do it. :)

@Faldrian Faldrian force-pushed the Faldrian:4055-add_contacts_mobile branch from 0463a56 to 8bddc48 Jan 26, 2015

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 26, 2015

Added tests, edited changelog and squashed into one commit.

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 27, 2015

The Hovercard-Test again... hmpf. Needs to be more robust.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jan 27, 2015

Hey, I tried to test that locally, so switching to the mobile version on Firefox desktop, and when I click on an aspect I got a popup with "Failed to add contact to aspect."

Here are the logs:

Started POST "/aspect_memberships" for 127.0.0.1 at 2015-01-27 09:55:57 +0100
Processing by AspectMembershipsController#create as JSON
  Parameters: {"person_id"=>2, "aspect_id"=>6, "aspect_membership"=>{"aspect_id"=>6}}
  ESC[1mESC[36mUser Load (0.8ms)ESC[0m  ESC[1mSELECT  "users".* FROM "users"  WHERE "users"."id" = 2  ORDER BY "users"."id" ASC LIMIT 1ESC[0m
  ESC[1mESC[35mPerson Load (0.4ms)ESC[0m  SELECT  "people".* FROM "people"  WHERE "people"."id" = $1 LIMIT 1  [["id", 2]]
  ESC[1mESC[36mAspect Load (0.5ms)ESC[0m  ESC[1mSELECT  "aspects".* FROM "aspects"  WHERE "aspects"."user_id" = $1 AND "aspects"."id" = 6  ORDER BY order_id ASC LIMIT 1ESC[0m  [["user_id", 2]]
  ESC[1mESC[35mContact Load (0.5ms)ESC[0m  SELECT  "contacts".* FROM "contacts"  WHERE "contacts"."user_id" = $1 AND "contacts"."person_id" = 2 LIMIT 1  [["user_id", 2]]
  ESC[1mESC[36mPerson Load (0.3ms)ESC[0m  ESC[1mSELECT  "people".* FROM "people"  WHERE "people"."id" = $1 LIMIT 1ESC[0m  [["id", 2]]
  ESC[1mESC[35mUser Load (0.5ms)ESC[0m  SELECT  "users".* FROM "users"  WHERE "users"."id" = $1 LIMIT 1  [["id", 2]]
  ESC[1mESC[36mBlock Exists (0.4ms)ESC[0m  ESC[1mSELECT  1 AS one FROM "blocks"  WHERE "blocks"."user_id" = $1 AND "blocks"."person_id" = 2 LIMIT 1ESC[0m  [["user_id", 2]]
  ESC[1mESC[35mContact Exists (0.4ms)ESC[0m  SELECT  1 AS one FROM "contacts"  WHERE ("contacts"."person_id" = 2 AND "contacts"."user_id" = 2) LIMIT 1
  Rendered text template (0.0ms)
Completed 409 Conflict in 17ms (Views: 0.5ms | ActiveRecord: 3.8ms)

Also, I think it would be nice to start breaking the mobile.js file in different files.

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 27, 2015

I will check that.

Breaking mobile.js in different files is another issue, you can open an issue for that. I also think we have to refactor some things for mobile.js, as there is no i18n support for mobile.js so far (and no backbone is used.... which is okay, because mobile should be a light interface, but also lacks the benefit of using all desktop-version goodies).

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jan 27, 2015

Breaking mobile.js in different files is another issue, you can open an issue for that.

Well, I do not ask you to break the existing file in that PR, just to put your code in "aspect_management.js" (or a better name) instead of mobile.js ;)

@Faldrian Faldrian force-pushed the Faldrian:4055-add_contacts_mobile branch 2 times, most recently from 2f2c652 to 1c2b5e6 Jan 27, 2015

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 27, 2015

@Flaburgan I moved it to profile_aspects.js (since we don't manage aspects, we have things related to aspects on the profile page... so I thought this name would be suitable).

Also thank you VERY much for your local test ... I am a bit ashamed, but I left some debugging hardcoded person-id in the code and it worked for me because my tests seems to use the same ids as my test accounts.... fixed that now.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jan 27, 2015

Confirmed, it now works. Thank you!

It would be nice if you can add some visual improvement to the aspect drop down to match the desktop view, especially to have it green when the contact is already in at least one aspect, an gray if he isn't.

@Faldrian Faldrian force-pushed the Faldrian:4055-add_contacts_mobile branch from 1c2b5e6 to 70a84db Jan 27, 2015

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 27, 2015

@Flaburgan I tested that yesterday... but I tried it again and now the dropdown will be green if the person is in aspects. There is no gradient, because select-fields seem not to support gradients as background so far. :)

I left the dropdown white (when no connection between accounts) for better readability on mobile displays / bad light conditions or something... more contrast. I think the difference to green will work well.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jan 27, 2015

No problem about the gradient. To add the caret would be nice though. I would also remove the float: right;

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 27, 2015

caret?

float: right is necessary, so the button appears on the right like in desktop view.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jan 27, 2015

Here is a caret: ▼ it's ▼

I personally find
capture du 2015-01-27 10 42 46

nicer than
capture du 2015-01-27 10 42 33

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 27, 2015

Look at the screenshots in the PR-description: There is a caret, added by the browser. It's implementation-specific if a caret is added.

I took the design from the desktop-layout and the select is aligned right - so this will be aligned right. It feld natural to expect it on the right when you have been using desktop version before.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jan 27, 2015

Hm, weird you have a caret with Firefox for Android and I don't have one with Firefox desktop. Anyway, this is not critical.

About the alignment, you have my opinion, so it will be up to others to decide :)

Apart from that, it's an awesome work @Faldrian thank you so much for working on this!

@marienfressinaud

This comment has been minimized.

Copy link
Contributor

marienfressinaud commented Jan 27, 2015

I also prefer the select on the left (so it is aligned with other information) but I agree with your argument to keep it on the right.

It is a minor problem anyway! ;)

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 27, 2015

@Flaburgan Firefox for desktop uses other styles than the mobile one. It also does not have a touch-friendly overlay when selecting the dropdown (like the one shown in 2nd screenshot).

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Feb 2, 2015

Please correct the Readme.md when merging... when I submitted this PR, it was mergeable, but if other PRs get merged first, the state changes. I can't keep the PR up to date (rebase every time develop gets a new line in Readme.md) - so I think you can do it when merging? :)

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Feb 3, 2015

Yeah, don't worry about changelog conflicts, those should be easy enough to solve for the reviewer ;)


function profileRefreshAspectDropdown() {
var covertext, numselected = $('#user_aspects option.selected').length;
if(numselected == 0) {

This comment has been minimized.

@svbergerem

svbergerem Feb 14, 2015

Member

please use === instead

covertext = t_addcontact;
} else {
$('#user_aspects').addClass('hasconnection');
if(numselected == 1) {

This comment has been minimized.

@svbergerem

svbergerem Feb 14, 2015

Member

please use === instead

padding: 3px;

&.hasconnection {
background-color: #C1ED94;

This comment has been minimized.

@svbergerem

svbergerem Feb 14, 2015

Member

Is a color in colors.css.scss which you could use here? If you aren't able to find one it would be great if you could add this color to the file.

- current_user.aspects.each do |aspect|
- if !contact.nil? && contact.in_aspect?(aspect)
- mid = contact.aspect_memberships.where(:aspect_id => aspect.id).first.id
%option{:value => aspect.id, 'data-name' => aspect.name, 'data-mid' => mid, :class => 'selected'}

This comment has been minimized.

@svbergerem

svbergerem Feb 14, 2015

Member

please use data-membership_id or something similar instead of data-mid.

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Feb 14, 2015

Added all requested changes and i18N and stuff. :)

@@ -10,6 +10,8 @@
= @person.name
%span.description
= @person.diaspora_handle
.useraspects

This comment has been minimized.

@jhass

jhass Feb 15, 2015

Member

Total nitpick, but most of our classes seem to use snake_case

Edit: Oh, is that some existing style actually? or did you just want a plain %div?

This comment has been minimized.

@Faldrian

Faldrian Feb 15, 2015

Contributor

I think at first I did not know what I wanted and I started with a div with a class. I have transformed it into a div, since the select itself has some id now and I don't need this anymore.

var person_id = $('#user_aspects').data('person-id');

function profileRefreshAspectDropdown() {
var covertext, numselected = $('#user_aspects option.selected').length;

This comment has been minimized.

@jhass

jhass Feb 15, 2015

Member

Actually, while I'm nitpicking on snake_case for naming things... ;P

This comment has been minimized.

@Faldrian

Faldrian Feb 15, 2015

Contributor

numselected -> num_selected

This comment has been minimized.

@jhass

jhass Feb 15, 2015

Member

cover_text, has_connection, list_cover

This comment has been minimized.

@Faldrian

Faldrian Feb 15, 2015

Contributor

done

// remove from aspect
var membership_id = selected.data('membership_id');
$.ajax({
url: '/aspect_memberships/' + membership_id,

This comment has been minimized.

@jhass

jhass Feb 15, 2015

Member

I know this is basically nowhere done yet, but let's start using js-routes when we have it in the Gemfile already. You might need to add the require for mobile, I didn't check.

This comment has been minimized.

@Faldrian

Faldrian Feb 15, 2015

Contributor

done

@@ -0,0 +1,13 @@
- if user_signed_in? && @person != current_user.person
%select{:id => "user_aspects", 'data-person-id' => @person.id}

This comment has been minimized.

@jhass

jhass Feb 15, 2015

Member

Mmh, I wonder if this has to be an ID or if we can't have this reusable already for when somebody wants to add more than one dropdown to a single page, think contacts management/overview.

This comment has been minimized.

@Faldrian

Faldrian Feb 15, 2015

Contributor

changed to .user_aspects

@@ -0,0 +1,13 @@
- if user_signed_in? && @person != current_user.person

This comment has been minimized.

@jhass

jhass Feb 15, 2015

Member

Related to the last point, the if should probably be around the render call for the partial, since it's page specific, I think.

This comment has been minimized.

@Faldrian

Faldrian Feb 15, 2015

Contributor

done

= t("add_contact")
- contact = current_user.contact_for(@person)
- current_user.aspects.each do |aspect|
- if !contact.nil? && contact.in_aspect?(aspect)

This comment has been minimized.

@jhass

jhass Feb 15, 2015

Member

Don't !contact.nil? &&, just contact && or even contact.try(:in_aspect?, aspect)

This comment has been minimized.

@Faldrian

Faldrian Feb 15, 2015

Contributor

done

- contact = current_user.contact_for(@person)
- current_user.aspects.each do |aspect|
- if !contact.nil? && contact.in_aspect?(aspect)
- membership_id = contact.aspect_memberships.where(:aspect_id => aspect.id).first.id

This comment has been minimized.

@jhass

jhass Feb 15, 2015

Member

.limit(1).pluck(:id).first instead of .first.id, avoids instantiating the record just for a single attribute.

This comment has been minimized.

@Faldrian

Faldrian Feb 15, 2015

Contributor

done

And I sign in as "bob@bob.bob" on the mobile website

Scenario: verify different states of the cover button
When I visit the user profile "alice"

This comment has been minimized.

This comment has been minimized.

@Faldrian

Faldrian Feb 15, 2015

Contributor

removed

@@ -1106,6 +1106,8 @@ en:
shared:
aspect_dropdown:
add_to_aspect: "Add contact"
mobile_row_checked: "✓ %{name} (remove)"
mobile_row_unchecked: "– %{name} (add)"

This comment has been minimized.

@svbergerem

svbergerem Feb 15, 2015

Member

@jhass What do you think about removing '✓' and '-' from those translations and adding them via css ( option.selected::before { content: '✓'; }) instead?

This comment has been minimized.

@jhass

jhass Feb 15, 2015

Member

Sounds like a fairly sane thing to do

This comment has been minimized.

@Faldrian

Faldrian Feb 15, 2015

Contributor

Have already checked. This works on desktop browsers, but in mobile browsers the dropdown is rendered as popup-modal-dialog on your phone and that dialog does not respect any css or anything other than "this is the utf-8 text of the option". Not even background-color or anything. So ::before won't work.

This comment has been minimized.

@svbergerem

svbergerem Feb 15, 2015

Member

In that case you could add the additional char in app/views/aspect_memberships/_aspect_membership_dropdown.mobile.haml and app/assets/javascripts/mobile/profile_aspects.js. I just think those chars shouldn't be in the translated string.

This comment has been minimized.

@Faldrian

Faldrian Feb 15, 2015

Contributor

will do. My thought was just "if we do string concatenation, do it all at once".

@Faldrian Faldrian force-pushed the Faldrian:4055-add_contacts_mobile branch 2 times, most recently from cb5633c to 79cd5fd Feb 15, 2015

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Feb 15, 2015

I rewrote some of the code...

  • can be used multiple times on a page (just render the template multiple times with different ids as parameter)
  • this instance still has #user_aspects since cucumber can not test with just using a class

the code is now much cleaner and "self-contained" in a way that it does not rely on there being only this one instance. :)

@Faldrian Faldrian force-pushed the Faldrian:4055-add_contacts_mobile branch from 79cd5fd to dacbafe Feb 15, 2015

@@ -33,3 +33,7 @@
When /^I open the drawer$/ do
find('#menu_badge').click
end

Then /^the aspect dropdown should be labeled "([^"]*)"/ do |label|
expect(page).to have_css("#user_aspects option.list_cover", :text => label)

This comment has been minimized.

@jhass

jhass Feb 15, 2015

Member

This is where you couldn't use the class?

This comment has been minimized.

@Faldrian

Faldrian Feb 15, 2015

Contributor

No, there I could have used .user_aspects. This is where I could not use the class: https://github.com/diaspora/diaspora/pull/5594/files#diff-07ff6d8f76a7cabb01ec40c8990d498bR19

This comment has been minimized.

@jhass

jhass Feb 15, 2015

Member

That ends up here https://github.com/diaspora/diaspora/blob/develop/features/step_definitions/web_steps.rb#L75

Let's check the docs, so we can select it by its ID, name and label text. ID falls short, that needs to be unique inside the entire page, leaving us with name and label text. We have no label, so we're left with name. When we look at the step definition again, we can notice that it allows us to scope the lookup into a parent container. Seems like a reasonable alternative to make this fully reusable.

This comment has been minimized.

@jhass

jhass Feb 15, 2015

Member

Oh and btw., I wouldn't have any objections to have equivalents to https://github.com/diaspora/diaspora/blob/develop/features/step_definitions/aspects_steps.rb#L55 etc. for the mobile version of the aspect dropdown.

This comment has been minimized.

@Faldrian

Faldrian Feb 15, 2015

Contributor

used name and added "within" to have a reusable step :)

@@ -11,3 +11,6 @@
find('.delete', :match => :first).click
end

When(/^I visit the user profile "([^"]*)"/) do |userhandle|
visit("/u/#{userhandle}")
end

This comment has been minimized.

@jhass

jhass Feb 15, 2015

Member

Didn't we want to replace this step with the existing one?

- if contact.try(:in_aspect?, aspect)
- membership_id = contact.aspect_memberships.where(:aspect_id => aspect.id).limit(1).pluck(:id).first
%option{:value => aspect.id, 'data-name' => aspect.name, 'data-membership_id' => membership_id, :class => 'selected'}
= "" + t('shared.aspect_dropdown.mobile_row_checked', {:name => aspect.name} )

This comment has been minimized.

@jhass

jhass Feb 15, 2015

Member

Let's use string interpolation here, "✓ #{t('shared.aspect_dropdown.mobile_row_checked', name: aspect.name)}"

@Faldrian Faldrian force-pushed the Faldrian:4055-add_contacts_mobile branch from dacbafe to 8f3c03e Feb 15, 2015

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Feb 15, 2015

Fixed that, thanks for the advice. :)

@jhass jhass added this to the next-major milestone Feb 15, 2015

jhass added a commit that referenced this pull request Feb 15, 2015

@jhass jhass merged commit 34ad598 into diaspora:develop Feb 15, 2015

1 of 2 checks passed

continuous-integration/travis-ci The Travis CI build failed
Details
hound Hound has reviewed the changes.
@jhass

This comment has been minimized.

Copy link
Member

jhass commented Feb 15, 2015

Okay, thank you!

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Feb 15, 2015

THIS IS AWESOME!

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Feb 15, 2015

@Faldrian can you indicate on bounty source that you fixed the issue?

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Feb 15, 2015

You can claim the bounty here

@goobertron

This comment has been minimized.

Copy link

goobertron commented Feb 17, 2015

Great work, Faldrian (and commenters) - thanks very much!

@Faldrian Faldrian deleted the Faldrian:4055-add_contacts_mobile branch Feb 18, 2015

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