-
Notifications
You must be signed in to change notification settings - Fork 252
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 Voting Power Slider #798
Conversation
Sekhmet
commented
Oct 14, 2017
•
edited
Loading
edited
- Voting Power settings.
- Save settings with Save button.
- Add Voting Power slider depending on settings.
- Handle auto Voting Power.
- Load previous vote value on that post (if any) and use it as initial value.
- Add estimated vote value.
- Default slider value.
I don't think we need the option |
It's still WIP. |
Settings UI has been updated to add Voting Power + added Save button. Any reviews, ideas, problems so far? |
Wow! there is even tests, great work @Sekhmet! 🥇 Looking good for me. |
I have just 1 feedback, for the english text, should we talk about "upvote" or "like" ? "Voting power" or "Liking power"? ok maybe the last one is too much |
I think we should use like/dislike across our app. I would use |
Fixes issue with images not showing up inside storybook
|
||
export const GET_TRENDING_TOPICS = '@app/GET_TRENDING_TOPICS'; | ||
export const GET_TRENDING_TOPICS_START = '@app/GET_TRENDING_TOPICS_START'; | ||
export const GET_TRENDING_TOPICS_SUCCESS = '@app/GET_TRENDING_TOPICS_SUCCESS'; | ||
export const GET_TRENDING_TOPICS_ERROR = '@app/GET_TRENDING_TOPICS_ERROR'; | ||
|
||
export const GET_REWARD_FUND = '@app/GET_REWARD_FUND'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that we are having to recreate these action types a lot, it might be good to have a helper like what I did here:
busy/src/helpers/stateHelpers.js
Line 106 in 27c72a6
export const createAsyncActionType = type => ({ |
We can probably go back and refactor the other action types in a seperate PR, keep the lines of code in each file smaller too
formatTip = value => ( | ||
<div> | ||
<FormattedNumber | ||
style="percent" // eslint-disable-line react/style-prop-object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kinda weird that they chose to use the style
prop for this, but I guess thats what the library says to do (https://github.com/yahoo/react-intl/wiki/Components#formattednumber)
style="percent" // eslint-disable-line react/style-prop-object | ||
value={value / 100} | ||
/> | ||
<span style={{ opacity: '0.5' }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is inline styles better here than using a className and applying styles from there? I have no preference, just wondering, when to use inline styles vs classNames.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use this style quite a lot, so I think we should create global translucentText
class for this property and use it everywhere it's needed.
src/components/Slider/Slider.less
Outdated
text-align: center; | ||
|
||
&__amount { | ||
font-family: "Source Sans Pro", monospace; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be good to use one of the @font-family
variables in custom.less or create a new one
src/components/Story/Story.js
Outdated
switch (key) { | ||
case 'follow': | ||
this.props.onFollowClick(this.props.post); | ||
this.props.onFollowClick(post); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to use return
here vs break
as in all other ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No good reason, it was this way before this PR. It's not big change so I will change this here.
@@ -81,36 +98,57 @@ class Story extends React.Component { | |||
let followText = ''; | |||
|
|||
if (postState.userFollowed && !pendingFollow) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a lot of logic here in the render function, it might be a good idea to create a helper function within this component called this.getFollowText()
or something and have all the logic in there rather then in the render function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I agree with this I don't think that it's related to this PR and working on multiple unrelated pieces in the same PR shouldn't be encouraged. I don't think we should rewrite half of our code base while adding one feature. I think we should work on this in new PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah good idea, this PR is already pretty big
{saving ? <Icon type="loading" /> : <i className="iconfont icon-write" />} | ||
<FormattedMessage id="edit_post" defaultMessage="Edit post" /> | ||
</PopoverMenuItem>]; | ||
popoverMenu = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here, looks like a lot of logic in this render function, it might be good to break it down into its own function maybe this.renderPopoverMenu()
, also same with the rebloggedUI logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I agree with this I don't think that it's related to this PR and working on multiple unrelated pieces in the same PR shouldn't be encouraged. I don't think we should rewrite half of our code base while adding one feature. I think we should work on this in new PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had small comments, but looks like they were fixed, and we can do other comments in other PR.
Been playing around with this, seems to be working. great work! It will be good to see this live
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️