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 answer poll counts #6641

Merged
merged 1 commit into from
Jan 26, 2016
Merged

Conversation

Flaburgan
Copy link
Member

'cause my head doesn't want to calculate it anymore.

one: "1 vote so far"
other: "<%=count%> votes so far"
answer_count:
zero: "0 vote"
Copy link
Member Author

Choose a reason for hiding this comment

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

zero doesn't work. How to do that?

Copy link
Member

Choose a reason for hiding this comment

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

The english pluralization rule doesn't make use of zero. And 0 votes is in fact the correct English.

Copy link
Member Author

Choose a reason for hiding this comment

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

But it will still be presented to translators in webtranslateit automatically right?
Good then, I'll revert that.

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

@jaywink
Copy link
Contributor

jaywink commented Jan 15, 2016

Nice 👍

@gtcarlos
Copy link

👍

@Flaburgan
Copy link
Member Author

I need a little help with the failing Jasmine test. The line expect(this.view.$('.poll_progress_bar_wrapper:first').css('display')).toBe("none"); is failing now on Travis but is green locally. This PR changes the markup removing style="display: none;" and adding it to the CSS file. Looks like .css('display') on Travis isn't able to get the result from the CSS file. Any idea?

@svbergerem
Copy link
Member

Add the rendered element to the spec content and test the visibility there:

diff --git a/spec/javascripts/app/views/poll_view_spec.js b/spec/javascripts/app/views/poll_view_spec.js
index 0b9b7d5..bbd4551 100644
--- a/spec/javascripts/app/views/poll_view_spec.js
+++ b/spec/javascripts/app/views/poll_view_spec.js
@@ -2,7 +2,7 @@ describe("app.views.Poll", function(){
   beforeEach(function() {
     loginAs({name: "alice", avatar : {small : "http://avatar.com/photo.jpg"}});
     this.view = new app.views.Poll({ model: factory.postWithPoll()});
-    this.view.render();
+    spec.content().html(this.view.render().el);
   });

   describe("setProgressBar", function(){
@@ -15,9 +15,9 @@ describe("app.views.Poll", function(){

   describe("toggleResult", function(){
     it("toggles the progress bar and result", function(){
-      expect(this.view.$('.poll_progress_bar_wrapper:first').css('display')).toBe("none");
-      this.view.toggleResult(null);
-      expect(this.view.$('.poll_progress_bar_wrapper:first').css('display')).toBe("block");
+      expect($(".poll_progress_bar_wrapper:first")).toBeHidden();
+      this.view.toggleResult();
+      expect($(".poll_progress_bar_wrapper:first")).toBeVisible();
     });
   });

You can run the jasmine tests locally with

bin/rake jasmine:ci

instead of running them in the browser. Travis is doing the same thing.

@@ -89,7 +89,7 @@ app.views.Poll = app.views.Base.extend({
},

toggleElements: function() {
this.$('.percentage').toggle();
this.$('.poll-result').toggle();

Choose a reason for hiding this comment

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

Strings must use doublequote.


input[type=radio],
label {
vertical-align: middle;

Choose a reason for hiding this comment

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

Properties should be ordered font-weight, margin-bottom, vertical-align

@@ -300,5 +300,8 @@ en:
count:
one: "1 vote so far"
other: "<%=count%> votes so far"
answer_count:
one: "1 vote"
Copy link
Member Author

Choose a reason for hiding this comment

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

I still have a bug here, looks like the "0" case is using the "one" translation. Any idea what's going on?

Copy link
Member

Choose a reason for hiding this comment

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

Other translations in that file look like this:

reshares:
  zero: "<%= count %> Reshares"
  one: "<%= count %> Reshare"
  other: "<%= count %> Reshares"

I'm not familiar with our translation code but I'd try to do the same here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jhass told me a few days ago that zero was not needed for english and the "count" translation just above doesn't have it and works well, so really no idea where the problem is.

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad, the bug was there because the local was set to french, which needs a zero key. So, no bug.

Copy link
Member

Choose a reason for hiding this comment

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

maybe you should add zero to the english translation, even if english doesn't need it, but other languages use this key as fallback.

Copy link
Member Author

Choose a reason for hiding this comment

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

@SuperTux88 I did that first but then removed it following @jhass advice. Jonne what should we do here? (even if this doesn't block this PR so is maybe a out of topic discussion).

Copy link
Member

Choose a reason for hiding this comment

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

Add zero.

@Flaburgan
Copy link
Member Author

Ok, should be ready to merge ;)

@svbergerem svbergerem merged commit 1e27b50 into diaspora:develop Jan 26, 2016
svbergerem pushed a commit that referenced this pull request Jan 26, 2016
@svbergerem svbergerem added this to the 0.6.0.0 milestone Jan 26, 2016
@svbergerem
Copy link
Member

Seems legit. Thank you!

svbergerem pushed a commit that referenced this pull request Jan 26, 2016
@Flaburgan Flaburgan deleted the add-poll-result branch January 27, 2016 07:45
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.

8 participants