-
Notifications
You must be signed in to change notification settings - Fork 379
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
Use mobx for follow user button #11216
Use mobx for follow user button #11216
Conversation
super(props); | ||
makeObservable(this); | ||
|
||
this._following = this.props.following; |
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.
as this is the only place using this props, might as well remove the default value and make it optional?
looking again, none of the ones using it passes the value so just always init to true? Or put the actual state on page so it can be restored on navigation
following: core.currentUserModel.following.has(this.props.follow.notifiable_id), | ||
}); | ||
} | ||
this.xhr?.abort(); |
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.
return instead?
}; | ||
private get following() { | ||
return this.props.follow.subtype === 'mapping' | ||
? core.currentUserModel.following.has(this.props.follow.notifiable_id) |
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.
the field name should be something like followUserMapping
instead of following
...
|
||
private readonly eventId = `follow-toggle-${nextVal()}`; | ||
private toggleXhr: null | JQueryXHR = null; | ||
@observable _following: boolean; |
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.
private
} | ||
|
||
componentDidMount() { | ||
if (this.props.follow.subtype === 'mapping') { | ||
$.subscribe(`user:followUserMapping:refresh.${this.eventId}`, this.refresh); |
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 doesn't seem to be anything using this event anymore so the publish can be removed
@observer | ||
export default class FollowToggle extends React.PureComponent<Props> { | ||
@observable private _following = true; | ||
private readonly jsonId = `json-follow-toggle-${this.props.follow.subtype}-${this.props.follow.notifiable_type}-${this.props.follow.notifiable_id}`; |
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.
this should be a getter? doesn't matter now but it'll probably break if the component is ever shuffled around and get passed a different set of props
Also fix the
being read outside a reactive context
warning on user card tooltips