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

Fix bug where singular values get plural endings #2069

Merged

Conversation

@selbekk
Copy link
Contributor

commented Mar 15, 2019

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

I've added two ternaries to add a trailing s to "comment" and "reaction" if and only if the count is not 1.

Related Tickets & Documents

#2067

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

relieved

@CLAassistant

This comment has been minimized.

Copy link

commented Mar 15, 2019

CLA assistant check
All committers have signed the CLA.

@@ -28,7 +28,7 @@
<% end %>
<% if article.published %>
<span id="pageviews-<%= article.id %>" class="cta pill dashboard-pageviews-indicator" data-analytics-pageviews data-article-id="<%= article.id %>">
<%= article.page_views_count > 100 ? article.page_views_count : "under 100" %> views // <%= article.positive_reactions_count %> reactions // <%= article.comments_count %> comments
<%= article.page_views_count > 100 ? article.page_views_count : "under 100" %> views // <%= article.positive_reactions_count %> reaction<%= article.positive_reactions_count != 1 ? "s" : "" %> // <%= article.comments_count %> comment<%= article.comments_count != 1 ? "s" : "" %>

This comment has been minimized.

Copy link
@rhymes

rhymes Mar 15, 2019

Collaborator

Hi @selbekk, good catch, you could consider using pluralize instead of manual counts

This comment has been minimized.

Copy link
@selbekk

selbekk Mar 15, 2019

Author Contributor

I could, but I have no ruby experience at all, so I’m not sure how i’d do that

This comment has been minimized.

Copy link
@Link2Twenty

Link2Twenty Mar 15, 2019

Contributor

@rhymes is textHelper already imported?

This comment has been minimized.

Copy link
@abraham

abraham Mar 15, 2019

Contributor

pluralize takes an count as and the singular string like this pluralize(2, 'person') and then will return the correct (usually) pluralization of the word people.

In this case the count is a dynamic value so it would look like this

pluralize(article.positive_reactions_count, 'reaction')

I then needs to be interpolated in the HTML template so you can wrap it in <%= %>

<%= pluralize(article.positive_reactions_count, 'reaction') %>

This comment has been minimized.

Copy link
@Link2Twenty

Link2Twenty Mar 15, 2019

Contributor

Interestingly, there's not really much in it, apart from style of course.

<%= pluralize(article.positive_reactions_count, "reaction") %>
reaction<%= article.positive_reactions_count != 1 ? "s" : "" %>
reaction<%= article.positive_reactions_count != 1 && "s" %>

This comment has been minimized.

Copy link
@rhymes

rhymes Mar 15, 2019

Collaborator

@selbekk no worries, I can explain: basically pluralize is a Rails helper, a method injected in the template view to help formatting. It works with two parameters, the first is the count and the second is the string that has the possible plural, in our case:

  • article.positive_reactions_count for "reaction"
  • article.comments_count for the word "comment"

To use a helper you call it inside the "outputting" tags, for example, instead of

reaction<%= article.positive_reactions_count != 1 ? "s" : "" %>

you could call

<%= pluralize(article.positive_reactions_count, "reaction") %>

This way the framework will decide to use the singular or plural form based from the value of count. The main difference are readability, the fact that you can use irregular forms or different words by explicitly passing the plural:

<%= pluralize(article.positive_reactions_count, "reaction", plural: "reactions") %>

and support for multiple locale (not a concern in this case I think)

Hope this helps :)

This comment has been minimized.

Copy link
@abraham

abraham Mar 15, 2019

Contributor

Rails overloads String with pluralize so you can also do 'reaction'.pluralize(count: 2).

This comment has been minimized.

Copy link
@Link2Twenty

Link2Twenty Mar 15, 2019

Contributor
<%= "reaction".pluralize(count: article.positive_reactions_count) %>

@selbekk selbekk force-pushed the selbekk:bug/make-endings-singular branch from 939add8 to 2bc8a61 Mar 15, 2019

@selbekk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

Thanks for all the help and fantastic explanations. Amended the commit to use pluralize :)

@maestromac maestromac merged commit f352344 into thepracticaldev:master Mar 19, 2019

7 checks passed

Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
codeclimate All good!
Details
codeclimate/diff-coverage 100% (50% threshold)
Details
codeclimate/total-coverage 89% (0.0% change)
Details
deploy/netlify Deploy preview ready!
Details
license/cla Contributor License Agreement is signed.
Details

@pr-triage pr-triage bot added the PR: merged label Mar 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.