Fix: Prevent NaN error in Campaign Overview Stat Widget #7698
Conversation
| @@ -93,7 +93,7 @@ const StatWidget = ({label, values, description, formatter = null}) => { | |||
| </header> | |||
| <div className={styles.statWidgetAmount}> | |||
| <DisplayText> | |||
| {formatter?.format(values[0]) ?? values[0]} | |||
| {!isNaN(values[0]) ? formatter?.format(values[0]) ?? values[0] : (<span> </span>)} | |||
There was a problem hiding this comment.
@kjohnson can you help us understand what could cause one of these values to be considered NaN? Is it an issue with data still loading or is it possible for a value to resolve as a non number?
There was a problem hiding this comment.
The isssue here is that the data is loading. The endpoint would only return a numeric value.
There was a problem hiding this comment.
Okay, If that's the case then I feel it would be more sensible for the stat widget to have a loading state (or conditionally rendered all together). You could then add a css class for the loading state to be styled.
Without the loading state we are making the assumption the value is always resolved when passed to the component, which is not true here - raising questions of why it could be a non number.
There was a problem hiding this comment.
This passes a non-breaking space as a placeholder if the value is not provided, which is during loading. While not a proper loading state, this does cut down on the amount of visual change while loading. I think it would be more distracting for a row of widgets to flash loading spinners or something.
Note that I did update the check from a NaN check to an undefined check, see d3077e7.
There was a problem hiding this comment.
Regardless of the design, what is the reason for not having a loading state in the logic?
Loading states tell us when our data is resolved and ready to be used. They are useful in the application's logic for conditionally rendering data. In this scenario we have data that is not resolved yet but we are attempting to render it regardless. There's lots of ways we can go about this visually but to me the important part is we are being intentional about our data loading and rendering.
There was a problem hiding this comment.
I really don't think this needs a special loading state. It is a simple widget which shows a number returned by the server. We aren't running exspensive queries and we aren't loading tables of data. There isn't a fancy display to be stubbed by a skeleton.
This PR prevents a flashed NaN and additionally prevents a content shift when the data is loaded.
There was a problem hiding this comment.
Circling back on this, the point was raised that this bug would have been prevented if we had accounted for loading state higher up in the flow of data. While I agree with this point, my goal with this PR was to immediately address the issue of the flashed NaN bug as it was presented and refined by the team in accordance with the allotted time of the development cycle. Being that there has been feedback as to how to improve the flow of data, it is my intention that this component architecture be updated at a later time. The intent of which is to balance the cadence of addressing reported issues with the deferred cost of accrued technical debt while meeting negotiated timelines.
To move this work forward it is my suggestion that we continue with the check for undefined data within the nested component while further discussing the best approach for handling undefined data in a subsequent pull request.
There was a problem hiding this comment.
@jonwaldstein @glaubersilva any other thoughts?
There was a problem hiding this comment.
@kjohnson that plan sounds good to me, thanks!
There was a problem hiding this comment.
@kjohnson It's a good point! Let's revisit it later.
|
Build for QA, see https://github.com/impress-org/givewp/actions/runs/13139955039. |
Resolves GIVE-2031
Description
This PR updates the
<StatWidget>component to prevent aNaNerror while fetching data.Additionally, a non-breaking space is used to prevent content shifting.
Visuals
Testing Instructions
Pre-review Checklist
@unreleasedtags included in DocBlocks