FlatTermSelector: Set Correct Loading State

[tfrommen]

Description

If the FlatTermSelector component mounts with one or more terms provided, it does not correctly set the internal loading state.

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 loading state when mounting.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[jorgefilipecosta]

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.

[tfrommen]

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). 🤔 🤷‍♂️

[jorgefilipecosta]

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.

[tfrommen]

@jorgefilipecosta updated as per your feedback.

[jorgefilipecosta]

LGTM 👍 Thank you for iteracting on this @tfrommen!