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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't display '0 reactions' on article feed #12425

Merged
merged 9 commits into from Jan 27, 2021

Conversation

msarit
Copy link
Contributor

@msarit msarit commented Jan 25, 2021

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

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

View updated RRC here.
Do not display 0 reactions on the article feed card, when that article has zero reactions.

Related Tickets & Documents

None

QA Instructions, Screenshots, Recordings

=> Implemented this change in 3 views; be sure to QA them all 馃檹馃従

dont-display-zero-reactions

UI accessibility concerns?

None

Added tests?

  • Yes
  • No, and this is why: please replace this line with details on why tests
    have not been included
  • I need help with writing tests

Added to documentation?

[optional] Are there any post deployment tasks we need to perform?

None

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

Seth Meyers taking test

@pr-triage pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label Jan 25, 2021
@vaidehijoshi vaidehijoshi requested a review from a team January 25, 2021 18:13
@msarit msarit changed the title Don't display '0 reactions' on article feed [WIP] Don't display '0 reactions' on article feed Jan 26, 2021
@msarit msarit added the merge by any core This tag is a signal that you are okay with any core team member merging your code for you. label Jan 26, 2021
Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

The tests seem fine!

<span className="hidden s:inline">
&nbsp;
{`${totalReactions == 1 ? 'reaction' : 'reactions'}`}
if (totalReactions > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be wrong here but I think you can return nothing immediately at the top of the function if the count is 0 because the rest of the component is just about rendering 1 or plus as the count

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense 馃槃 I will modify

Comment on lines 9 to 10
import '../../../../assets/javascripts/lib/xss';
import '../../../../assets/javascripts/utilities/timeAgo';
Copy link
Contributor

Choose a reason for hiding this comment

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

are these needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

馃槄 you're right. I copied my test from the Article.test.jsx; these imports are the remnants of that move. I shall delete.

@msarit msarit marked this pull request as ready for review January 26, 2021 16:14
@pr-triage pr-triage bot removed the PR: draft bot applied label for PR's that are a work in progress label Jan 26, 2021
@msarit msarit changed the title [WIP] Don't display '0 reactions' on article feed Don't display '0 reactions' on article feed Jan 26, 2021
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 26, 2021
Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

Works well!

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 26, 2021
@nickytonline nickytonline removed their request for review January 26, 2021 18:37
Copy link
Contributor

@mstruve mstruve left a comment

Choose a reason for hiding this comment

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

馃殌 AWESOME!

@msarit msarit merged commit 23f3386 into master Jan 27, 2021
@msarit msarit deleted the msarit/dont-display-zero-reactions-on-feed branch January 27, 2021 16:13
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge by any core This tag is a signal that you are okay with any core team member merging your code for you. PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants