FlatTermSelector: Set Correct Loading State#13758
FlatTermSelector: Set Correct Loading State#13758jorgefilipecosta merged 2 commits intoWordPress:masterfrom
Conversation
tfrommen
requested review from
gziolo,
noisysocks,
oandregal,
talldan and
youknowriad
as code owners
gziolo
added
General Interface
| componentDidMount() { | ||
| if ( ! isEmpty( this.props.terms ) ) { | ||
| this.setState( { loading: false } ); | ||
| this.setState( { loading: true } ); |
There was a problem hiding this comment.
Hi @tfrommen thank you for your PR. Would it work if we set the loading state true when ! isEmpty( this.props.terms ) on the constructor instead of on componentDidMount? The reason being we would avoid an unnecessary rerender that happens when setting state via componentDidMount.
There was a problem hiding this comment.
Hi @jorgefilipecosta - hm, sure, I could do that. And I understand your reasoning.
On the other hand, it somehow feels a bit strange to split the actual loading logic (including the subsequent loading: false) from the part where we set the loading state to be true (in the constructor). 🤔 🤷♂️
There was a problem hiding this comment.
Hi @tfrommen,
On the constructor, we are initializing the state to false loading: false,. I don't think it is wrong to initialize to true or false depending on if this.props.terms is empty or not. If terms exist we know that the component will start the loading process.
Basically, we would set the loading on the constructor and unset the loading when the request is finished, I agree the code would look better if this logical parts were together but given the performance reason I think the separation is ok.
There was a problem hiding this comment.
@jorgefilipecosta updated as per your feedback.
jorgefilipecosta
left a comment
jorgefilipecosta
left a comment
There was a problem hiding this comment.
LGTM 👍 Thank you for iteracting on this @tfrommen!
3 participants
Description
If the
FlatTermSelectorcomponent mounts with one or more terms provided, it does not correctly set the internalloadingstate.How has this been tested?
Edit a post that has no tags assigned, and edit one that does have one tag assigned (see Screenshots).
Screenshots
Showing a post with one tag assigned.
Before:
After:
Types of changes
This change correctly sets the internal
loadingstate when mounting.Checklist: