🐛 Shows client side loading state for jira tracker #1409
Conversation
ibolton336
changed the title
ibolton336
force-pushed
the
mta-1019
branch
from
6e05cb0
to
e4c2be5
Compare
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1409 +/- ##
=======================================
Coverage 41.07% 41.07%
=======================================
Files 139 139
Lines 4404 4404
Branches 1010 1010
=======================================
Hits 1809 1809
Misses 2583 2583
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| connected={tracker.connected} | ||
| message={tracker.message} | ||
| /> | ||
| {updatingTrackerId === tracker.id ? ( |
There was a problem hiding this comment.
This will be "the most recently updated/created tracker". Might want to track an array of tracker ids just to be sure that you get a spinner on multiple trackers if they're both edited very close together.
Maybe a Set would work better? But then since a Set is mutable, getting useState() to track changes would be a bit of a challenge. Some stackoverflow discussion: https://stackoverflow.com/questions/58806883/how-to-use-set-with-reacts-usestate
There was a problem hiding this comment.
Converted to use a set internal to the hook but exported the readonly state as an array. Interesting on the mutable workaround. Updated the hook to use the approach you linked.
| {updatingTrackerIds.includes(tracker.id) ? ( | ||
| <Spinner size="sm" /> | ||
| ) : ( | ||
| <TrackerStatus | ||
| name={tracker.name} | ||
| connected={tracker.connected} | ||
| message={tracker.message} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
I don't mind this setup, but you could push the spinner in to TrackerStatus and just set a prop isTrackerUpdating={updatingTrackerIds.includes(tracker.id)}. That way, all of the render code for the column is in a single component.
| if (updatingTrackerIds.size > 0) { | ||
| timerId = window.setTimeout(() => { | ||
| setUpdatingTrackerIds(new Set()); | ||
| }, delay); | ||
| } | ||
|
|
||
| return () => { | ||
| if (timerId !== null) { | ||
| window.clearTimeout(timerId); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Does this work if you updated 2 trackers within the 10 second interval?
The addUpdatingTrackerId function looks right, but the removal of the ids seems off since I don't see something like new Set(prevSet.remove(expiredId)).
There was a problem hiding this comment.
It is actually working because on refetch this hook is remounted / cleaned up. Only works because we are routinely polling for trackers.
There was a problem hiding this comment.
Ah I see what you mean. The entire set is wiped when one timeout expires. Fixing.
… instance Signed-off-by: ibolton336 <ibolton@redhat.com>
Signed-off-by: ibolton336 <ibolton@redhat.com>
ibolton336
force-pushed
the
mta-1019
branch
2 times, most recently
from
eab8b43
to
07c96b1
Compare
Signed-off-by: ibolton336 <ibolton@redhat.com>
Closes False 'not connected' status for new Jira instance