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

issue #4888: show user's vote in poll #7550

Closed

Conversation

@netagonen
Copy link
Contributor

netagonen commented Aug 17, 2017

Hi!

Just wanted to let you know that we didn't fix the following checks for the reasons stated below them:

app/assets/javascripts/app/views/poll_view.js:36 W: Identifier 'original_post_link' is not in camel case.
app/assets/javascripts/app/views/poll_view.js:37 W: Identifier 'poll_answers' is not in camel case.

Here we are following exactly the code that is already there and it seems like it is properly formatted.

app/models/poll.rb:28 W: Use the new Ruby 1.9 hash syntax.
app/models/poll.rb:28 W: Redundant `self` detected.

These are not changes that we have made.

CC @Rete2

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 19, 2017

Please fix (some of) the code style errors. We still have a lot of old code that doesn't follow our styleguides and we agreed on changing the code to follow the styleguides whenever we touch it.

When fixing the camel case issues you would also have to change the identifiers in the poll template. If you don't want to do that, then I'd say it's okay to leave the code like it is.

The ruby code style issues in app/models/poll.rb should be fixed.

You will also have to add a fix for the dark theme (in vim app/assets/stylesheets/color_themes/_color_theme_override_dark.scss).

poll dark theme

@netagonen

This comment has been minimized.

Copy link
Contributor Author

netagonen commented Aug 21, 2017

When fixing the camel case issues you would also have to change the identifiers in the poll template. If you don't want to do that, then I'd say it's okay to leave the code like it is.

We changed the camel case also in the template.

The ruby code style issues in app/models/poll.rb should be fixed.

We changed lines 24-28 the the new hash syntax and not only line 28.
Regarding the Redundant 'self': what does that mean?

@SuperTux88

This comment has been minimized.

Copy link
Member

SuperTux88 commented Aug 21, 2017

Regarding the Redundant 'self': what does that mean?

That means, that you don't need self there, you can simply remove it.

@netagonen netagonen force-pushed the codebearsteam:4888-show-user-vote-on-poll branch from 6ab5b5e to 937f5ab Aug 21, 2017
@netagonen

This comment has been minimized.

Copy link
Contributor Author

netagonen commented Aug 21, 2017

Hi, we fixed the style errors. BUT we are still working on adding a label next to the progress-bar, so it's not ready for a full review.

@SuperTux88 SuperTux88 changed the title issue #4888: show user's vote in poll [WIP] issue #4888: show user's vote in poll Aug 21, 2017
@SuperTux88

This comment has been minimized.

Copy link
Member

SuperTux88 commented Aug 21, 2017

By the way: You can add [WIP] (work in progress) to a PR in the title, so it's visible to us, that you are still working on it (but you want to ask a question or need some feedback about something). I added that now, you can remove it (and write a comment, because title changes don't send notifications) when you think you are finished and it's ready for a full review (only the person who opened the PR can edit the title).

@netagonen netagonen force-pushed the codebearsteam:4888-show-user-vote-on-poll branch from 937f5ab to 67c8fd8 Aug 22, 2017
@netagonen netagonen changed the title [WIP] issue #4888: show user's vote in poll issue #4888: show user's vote in poll Aug 22, 2017
@netagonen

This comment has been minimized.

Copy link
Contributor Author

netagonen commented Aug 22, 2017

We are now ready for a full review :)
We only got this error that we are not sure how to fix, can you give us a hint how to?
.eslintrc: F: Parsing error: Unexpected token }

if (defaultPresenter.poll && defaultPresenter.poll.poll_answers) {
var pollAnswers = defaultPresenter.poll.poll_answers;
pollAnswers = _.map(pollAnswers, function(answer) {
return _.extend(answer, {isCurrentUserVote: answer.id === answerGiven, currentUser});

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 22, 2017

Member

This line is causing the syntax error. It should be currentUser: currentUser.

(I'll do the full review later)

This comment has been minimized.

Copy link
@netagonen

netagonen Aug 22, 2017

Author Contributor

Thanks! fixed! now it's completely ready

@netagonen netagonen force-pushed the codebearsteam:4888-show-user-vote-on-poll branch from 67c8fd8 to 505f222 Aug 22, 2017
Copy link
Member

svbergerem left a comment

These are mostly small remarks. The new color for the progress bars looks good, but there is something about the avatar next to the label that I don't like. Unfortunately I'm currently unable to describe the reason for that. I tried to add some margin to the left of the avatar, but I'm still not satisfied. Maybe someone else has a good idea? I'll add a screenshot of the current solution below.


return _.extend(defaultPresenter, {
show_form: showForm,
is_reshare: isReshare,
original_post_link: originalPostLink
originalPostLink: originalPostLink,
pollAnswers: pollAnswers

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 22, 2017

Member

You don't use it in the poll template, so you can just remove it.

var pollAnswers = defaultPresenter.poll.poll_answers;
pollAnswers = _.map(pollAnswers, function(answer) {
return _.extend(answer, {isCurrentUserVote: answer.id === answerGiven, currentUser: currentUser});
});

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 22, 2017

Member

The only purpose of these four lines is changing defaultPresenter.poll.poll_answers and I'd like to make that more explicit. Maybe something like this:

defaultPresenter.poll.poll_answers.forEach(function (answer) {
  // set answer.isCurrentUserVote and answer.currentUser
});
</label>
{{else}}
<label>{{answer}}</label>
{{/if}}

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 22, 2017

Member

The indentation here is a bit weird. {{#if isCurrentUserVote}} should be indented like <input type="radio" name="vote" value="{{id}}"/> and both {{answer}} and {{#linkToAuthor currentUser}} like <label>.

{{answer}}
{{#linkToAuthor currentUser}}
{{{personImage this 'small' 'micro'}}}
{{/linkToAuthor}}

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 22, 2017

Member

I'm not sure if we should add a link to our own profile or just a simple avatar without the link.

var answer2 = this.view.poll.poll_answers[1];
this.view.model.set("poll_participation_answer_id", answer1.id);
var progressBarAnswer2 = ".poll_progress_bar[data-answerid='" + answer2.id + "']";
expect(this.view.$(progressBarAnswer2).hasClass("users-vote")).toBe(false);

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 22, 2017

Member

Using a variable for the selector looks unnecessary to me. You could replace progressBarAnswer2 by the value set before and still follow the maximum line length rule. (120 chars)

var answer = this.view.poll.poll_answers[0];
this.view.model.set("poll_participation_answer_id", answer.id);
expect(this.view.$(".poll_progress_bar.users-vote").css("background-color")).toBe("rgb(0, 136, 230)");
});

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 22, 2017

Member

You already tested before that the users-vote class is added, so here you only check the CSS rule for .users-vote. I'd say we can remove this test, since it should only test things our JavaScript code does. @denschub Any other opinion?

This comment has been minimized.

Copy link
@netagonen

netagonen Aug 23, 2017

Author Contributor

We removed the test

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 22, 2017

Here is a screenshot of the current design:

poll view avatar

@SuperTux88

This comment has been minimized.

Copy link
Member

SuperTux88 commented Aug 22, 2017

Hmm, I see what you mean, the avatar doesn't really fit there. While I somehow like it, it also looks strange.

Also: When a user doesn't have an avatar it would simply display the default fallback avatar, which isn't clear then. So maybe just a text label with "your vote" or something like this is better?

@Rete2

This comment has been minimized.

Copy link
Contributor

Rete2 commented Aug 23, 2017

We put a text label next to the answer now. For some reason I can´t post a screenshot here right now. I will try that later.
But one question about translations:
We added a translation in config/locales/javascript/javascript.en.yml and use it then in the template for the text in the label. Is this the right thing to do or do we need to add it in a different file?

@SuperTux88

This comment has been minimized.

Copy link
Member

SuperTux88 commented Aug 23, 2017

We added a translation in config/locales/javascript/javascript.en.yml and use it then in the template for the text in the label. Is this the right thing to do or do we need to add it in a different file?

Yes, that's the correct file.

@netagonen netagonen changed the title issue #4888: show user's vote in poll [WIP] issue #4888: show user's vote in poll Aug 23, 2017
@Rete2

This comment has been minimized.

Copy link
Contributor

Rete2 commented Aug 23, 2017

We´re still working on your comments, so no need to look at the latest commit yet.

@netagonen netagonen force-pushed the codebearsteam:4888-show-user-vote-on-poll branch from 1a0d631 to 2f9bd9e Aug 23, 2017
@netagonen netagonen changed the title [WIP] issue #4888: show user's vote in poll issue #4888: show user's vote in poll Aug 23, 2017
@netagonen

This comment has been minimized.

Copy link
Contributor Author

netagonen commented Aug 23, 2017

We made the corrections :) so it's ready on our part

@goobertron

This comment has been minimized.

Copy link

goobertron commented Aug 23, 2017

Sorry, I'm coming late to these latest changes.

I think the avatar is unnecessary: the blue colour of the bar indicates which option the user has voted for; and, as it is merely a reminder of which option they themselves voted for, I don't think it's necessary to reinforce this with their own avatar or a text label.

The avatar also doesn't really fit well in amongst the bars. It it a bit of visual 'noise'. While I haven't seen a screenshot of the text label there, I think this would have the same effect.

I think it's best to keep the design as simple as possible by just altering the bar colour.

@netagonen

This comment has been minimized.

Copy link
Contributor Author

netagonen commented Aug 23, 2017

At the moment we switched the avatar to a label. Originally we wanted this extra indication for the user's vote because a change of the progress bar color could be interpreted also as "the most popular vote" when it is actually the most popular vote. But if the majority here thinks it's unnecessary, we can drop this change.

@SuperTux88

This comment has been minimized.

Copy link
Member

SuperTux88 commented Aug 23, 2017

Here is a screenshot:

I think the label is OK (better than the avatar). I think it's cool to show what the color means. When the colored bar is also the winning one, I think it's maybe confusing if the color is because it's winning or because it's your vote. So I think a indicator would be nice. An alternative would be to add a tooltip when hovering the colored bar and show "your vote" there.

@@ -344,3 +344,4 @@ en:
other: "<%=count%> votes"
show_result: "Show result"
close_result: "Hide result"
vote_label: "Your vote"

This comment has been minimized.

Copy link
@SuperTux88

SuperTux88 Aug 23, 2017

Member

I think your_vote would be a better key than vote_label.

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 23, 2017

Member

I agree.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 23, 2017

There is some work to do to deal with long labels: #7573 this is not part of this PR, but if we add a label "Your vote" at the end, it will be even more common.


if (defaultPresenter.poll && defaultPresenter.poll.poll_answers) {
defaultPresenter.poll.poll_answers.forEach(function(answer) {
return _.extend(answer, {isCurrentUserVote: answer.id === answerGiven, currentUser: currentUser});

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 23, 2017

Member

You don't need the return here. Just write _.extend(...);.

<div class="poll_progress_bar_wrapper progress">
{{#if isCurrentUserVote}}
<label>
{{answer}}

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 23, 2017

Member

I'm sorry if I made a mistake in my comment before (I'm unable to check right now) but here are two whitespaces missing for indentation.

@@ -344,3 +344,4 @@ en:
other: "<%=count%> votes"
show_result: "Show result"
close_result: "Hide result"
vote_label: "Your vote"

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 23, 2017

Member

I agree.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Aug 23, 2017

Thanks for that screenshot. The label looks a lot better than I imagined; I thought it would be the user's name (which can be very long).

If everyone else wants a label, I'm happy for this to be used.

</div>
</div>

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 23, 2017

Member

Mhh, it looks like the indentation in this whole block needs some work.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 23, 2017

Let's start with the label. We can still change it in future commits if we think that there is a better solution, but for now I think it's ok.

@Rete2 Rete2 force-pushed the codebearsteam:4888-show-user-vote-on-poll branch from 2f9bd9e to ec0eab4 Aug 23, 2017
Copy link
Member

svbergerem left a comment

Just some cleanup after the latest changes. Everything else in the front-end looks good to me.

@@ -21,11 +21,19 @@ app.views.Poll = app.views.Base.extend({
var originalPostLink = isReshare && this.model.get('root') ?
'<a href="/posts/' + this.model.get('root').id + '" class="root_post_link">' + Diaspora.I18n.t('poll.original_post') + '</a>' :
'';
var answerGiven = this.model.get("poll_participation_answer_id");
var currentUser = defaultPresenter.current_user;

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 23, 2017

Member

You don't use currentUser anymore in the template, so you can remove it here.


if (defaultPresenter.poll && defaultPresenter.poll.poll_answers) {
defaultPresenter.poll.poll_answers.forEach(function(answer) {
_.extend(answer, {isCurrentUserVote: answer.id === answerGiven, currentUser: currentUser});

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 23, 2017

Member

And you can remove currentUser here as well.


.center-block {
display: inline;
}

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 23, 2017

Member

I think this rule is not needed anymore.

@netagonen netagonen force-pushed the codebearsteam:4888-show-user-vote-on-poll branch from ec0eab4 to 65e6559 Aug 23, 2017
Copy link
Member

svbergerem left a comment

The front-end code looks good to me. 👍

@svbergerem svbergerem requested a review from SuperTux88 Aug 24, 2017
@@ -47,6 +47,10 @@ def page_title
post_page_title @post
end

def poll_participation_answer_id
@post.poll.participation_answer(current_user).poll_answer_id if already_participated_in_poll

This comment has been minimized.

Copy link
@SuperTux88

SuperTux88 Aug 24, 2017

Member

You can combine this with already_participated_in_poll, so this line looks like this:

@post.poll&.participation_answer(current_user)&.poll_answer_id if user_signed_in?

and then also use poll_participation_answer_id to decide "if already participated" in the poll_view.js.

Then you can remove already_participated_in_poll from post_presenter.rb and remove already_participated? from poll.rb.

This has two reasons:

  1. less code
  2. less database queries

with the old way you queried the participation 3 times from the database:

  1. for the if already_participated_in_poll in this line
  2. for the poll.participation_answer(current_user) here
  3. again already_participated_in_poll to add it to non_directly_retrieved_attributes.

With the new line you do only one query, because you can decide "if already participated" if this method returns nil or the id.

poll_participation = status_message_with_poll.poll.poll_participations
poll_participation = poll_participation.create(poll_answer: poll_answer, author: alice.person)
expect(presenter.poll_participation_answer_id).to eql(poll_participation.poll_answer_id)
end

This comment has been minimized.

Copy link
@SuperTux88

SuperTux88 Aug 24, 2017

Member

When you do the change in post_presenter_spec.rb this would also need a negative test to test it returns nil when the user didn't participate yet.

This comment has been minimized.

Copy link
@Rete2

Rete2 Aug 24, 2017

Contributor

We did those changes and hope they are ok. Had to touch some other files now, but it was only because of the change of the method.

This comment has been minimized.

Copy link
@netagonen

netagonen Aug 24, 2017

Author Contributor

And thanks for all the explanations!

@svbergerem svbergerem added this to Waiting on contributor in Pull Requests Aug 24, 2017
@Rete2 Rete2 force-pushed the codebearsteam:4888-show-user-vote-on-poll branch from 65e6559 to 5dd7e7d Aug 24, 2017
Copy link
Member

SuperTux88 left a comment

Looks good, only one small thing.

@svbergerem can you have a quick look at the frontend-change again (replace already_participated_in_poll with poll_participation_answer_id)? But I think it's OK.

@@ -47,6 +47,10 @@ def page_title
post_page_title @post
end

def poll_participation_answer_id

This comment has been minimized.

Copy link
@SuperTux88

SuperTux88 Aug 24, 2017

Member

The only reason you made this method public is because you need it in the tests. But you shouldn't change code only for tests. Can you please move this method down in the private scope (for example where already_participated_in_poll was).

(sorry, I didn't see this yesterday night. just realized that this was already that way yesterday, but now after the refactorings it was more visible)

poll_answer = status_message_with_poll.poll.poll_answers.first
poll_participation = status_message_with_poll.poll.poll_participations
poll_participation = poll_participation.create(poll_answer: poll_answer, author: alice.person)
expect(presenter.poll_participation_answer_id).to eql(poll_participation.poll_answer_id)

This comment has been minimized.

Copy link
@SuperTux88

SuperTux88 Aug 24, 2017

Member

You can replace this with:

expect(presenter.to_json[:poll_participation_answer_id]).to eql(poll_participation.poll_answer_id)

it "returns nil if the user did not participate in a poll" do
presenter = PostPresenter.new(status_message_with_poll, alice)
expect(presenter.poll_participation_answer_id).to eql(nil)

This comment has been minimized.

Copy link
@SuperTux88

SuperTux88 Aug 24, 2017

Member

Same as above.

Copy link
Member

svbergerem left a comment

Two minor remarks, everything else looks good.

defaultAttrs = _.extend(defaultAttrs, {"already_participated_in_poll" : false});
defaultAttrs = _.extend(defaultAttrs, {"poll" : factory.poll()});
var defaultAttrs = _.extend(factory.postAttrs(), {"author": this.author()});
defaultAttrs = _.extend(defaultAttrs, {"poll_participation_answer_id": false});

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 24, 2017

Member

This should be null instead of false, because that's what the server will return.

@@ -65,7 +65,7 @@ describe("app.views.Poll", function(){
expect(this.view.$('form').length).toBe(1);
});
it("hides vote form when user voted before", function(){
this.view.model.attributes.already_participated_in_poll = true;
this.view.model.attributes.poll_participation_answer_id = this.view.poll.poll_answers[0].id;

This comment has been minimized.

Copy link
@svbergerem

svbergerem Aug 24, 2017

Member

You can replace this by

this.view.model.set("poll_participation_answer_id", this.view.poll.poll_answers[0].id);

to make pronto happy.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 24, 2017

I just found another small bug in the post preview while reviewing #7579:

poll preview

In the preview both the answer ids and the poll_participation_answer_id are undefined. We should only set isCurrentUserVote to true if the user gave an answer.

@Rete2 Rete2 force-pushed the codebearsteam:4888-show-user-vote-on-poll branch from 5dd7e7d to 1cd0da8 Aug 25, 2017
Copy link
Member

svbergerem left a comment

The front-end code looks good to me.

Copy link
Member

SuperTux88 left a comment

Backend looks good now.

@SuperTux88 SuperTux88 added this to the 0.7.1.0 milestone Aug 26, 2017
@SuperTux88

This comment has been minimized.

Copy link
Member

SuperTux88 commented Aug 26, 2017

Merged as b556ad5

Thank you @Rete2 and @netagonen 🍪 🍪

@SuperTux88 SuperTux88 removed this from Waiting on contributor in Pull Requests Aug 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.