[#8467] Allow moderators to like #5751
Conversation
sevfurneaux
force-pushed
the
sf-2024-allow-moderators-to-like
branch
from
3cfc4a4
to
61ef9bf
Compare
| useEffect(() => { | ||
| setisOnShortlist(props.is_on_shortlist) | ||
| }, [props.is_on_shortlist]) | ||
|
|
||
| useEffect(() => { | ||
| setIsLive(props.is_live) | ||
| }, [props.is_live]) | ||
|
|
||
| useEffect(() => { | ||
| setIsHidden(props.is_hidden) | ||
| }, [props.is_hidden]) | ||
|
|
||
| useEffect(() => { | ||
| setIsAnswered(props.is_answered) | ||
| }, [props.is_answered]) |
There was a problem hiding this comment.
Why are you doing it this way instead of using a callback provided by the parent component? If you need to update your props, you should use a callback and not copy props into state.
There was a problem hiding this comment.
Ah, I see, you just converted the old code. Could you investigate it still? Only if it doesn't take much time. The live-questions aren't really used that much.
There was a problem hiding this comment.
Why are you doing it this way instead of using a callback provided by the parent component?
@vellip I asked myself the same question of the original implementation, so I used all the props directly, without copying state. However doing this means on update of moderator buttons in this component, their interaction is delayed to the 500 ms polling when the parent props are updated. TLDR: the copied local state makes this faster.
If we can think of an alternative approach, happy to change it!
There was a problem hiding this comment.
Eh, then let's leave it in. Maybe you can add a small comment with your findings or link to this discussion.
There was a problem hiding this comment.
| is_answered: this.props.is_answered | ||
| } | ||
| } | ||
| const QuestionModerator = (props) => { |
There was a problem hiding this comment.
Maybe destructure the props argument
There was a problem hiding this comment.
@vellip Destructuring to this:
const QuestionModerator = ({
category,
children,
displayIsAnswered,
displayIsHidden,
displayIsLive,
displayIsOnShortlist,
handleLike,
id,
is_answered,
is_hidden,
is_live,
is_on_shortlist,
likes,
removeFromList,
togglePollingPaused,
updateQuestion
}) => {Causes eslint errors:
error Identifier 'is_answered' is not in camel case
But changing to isAnswered then conflicts with the isAnswered local state naming:
const [isAnswered, setIsAnswered] = useState(is_answered)
Let's keep as props for now?
There was a problem hiding this comment.
const QuestionModerator = ({
category,
children,
displayIsAnswered,
displayIsHidden,
displayIsLive,
displayIsOnShortlist,
handleLike,
id,
**is_answered: _isAnswered,**
is_hidden,
is_live,
is_on_shortlist,
likes,
removeFromList,
togglePollingPaused,
updateQuestion
}) => {does that work?
There was a problem hiding this comment.
| <button | ||
| type="button" | ||
| className={classNames( | ||
| 'cardbutton card__button card__button--live', |
There was a problem hiding this comment.
can you check if cardbutton is still needed? I didnt find any styling for it. It also doesn't look very BEM to me.
There was a problem hiding this comment.
cardbutton is from the compiled berlin_css.css which is part of the Card HTML in the styleguide: https://styleguide.berlin.de/patterns/09-vertical_marketing-page-modul-card/09-vertical_marketing-page-modul-card.html
sevfurneaux
force-pushed
the
sf-2024-allow-moderators-to-like
branch
from
61ef9bf
to
c7ad71f
Compare
Describe your changes
This PR refactors the
QuestionModeratorcomponent to a functional component, and also allows moderators to like alongside other users.Tasks