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

Fixed PropType warning and show all vote stats in modal #1084

Merged
merged 4 commits into from
Nov 17, 2017

Conversation

robert9g
Copy link
Contributor

Fixes #1080 .

Changes:

  • moved alt prop's elements from /components/Reactions/ReactionsList.js to /components/UserCard.js because of a PropType warning where alt prop was sent as object but a string type was expected (screenshot below)

  • removed the restriction to only show vote info for votes with the value greater than 0.1. This way users can see all the votes information, including the downvotes information (as reported in the 1080)

Screenshot of local (dev-server) PropType warning that comes once you click to show the votes modal:
votes-modal-warning-3

@bonustrack bonustrack temporarily deployed to busy-nd-pr-1084 November 16, 2017 15:16 Inactive
Copy link
Contributor

@Sekhmet Sekhmet left a comment

Choose a reason for hiding this comment

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

Hey,
we don't need to refactor UserCard to just fix that warning. We can set PropTypes to anything that makes sense from component's perspective.
To get rid of that warning we can just change PropTypes.bool (which was a mistake I guess) to PropTypes.element.
We shouldn't add any logic to UserCard. It is used in other places (followers/following list, and here vote value is not relevant). It should be as generic as possible.

@robert9g
Copy link
Contributor Author

I understand, thanks for letting me know. I will make the changes you asked.

@bonustrack bonustrack temporarily deployed to busy-nd-pr-1084 November 17, 2017 00:50 Inactive
@robert9g
Copy link
Contributor Author

robert9g commented Nov 17, 2017

Followers/following list raises an error it seems if there is just element as PropType, will add string too for the alt prop to avoid this.

@bonustrack bonustrack temporarily deployed to busy-nd-pr-1084 November 17, 2017 01:07 Inactive
@robert9g
Copy link
Contributor Author

robert9g commented Nov 17, 2017

Now I do not get any warnings, all logic is back to ReactionsList.js, no extra elements are added to followers/following and all vote info are present in the vote modal including downvotes. Hope it's ok now, let me know when you get a chance.

Copy link
Contributor

@Sekhmet Sekhmet left a comment

Choose a reason for hiding this comment

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

Yeah, I've meant to replace only bool with element, not everything.

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.

3 participants