-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add animated temporal progress bar #41
Conversation
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat solution.
Missed this in the last review, but I wonder about opening a follow-up card for adding some sort of accessibility feature for the coloring of the states, since it has semantic meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working well, but I think we can find a way to simplify how the interval updates. See review comments.
} | ||
}; | ||
} | ||
}, [playButtonDisabled, timeValue, spending]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because timeValue
is in the dependencies of this useEffect
, it will be reset each time timeValue
changes. You can confirm this by adding a console.log
to the clean up function. This means that the setInterval
only ever gets fired once before it's reset. If we want to continue with this strategy, we should use a useTimeout
. However, I like the idea of using an interval. It feels more declarative than waiting for a timeout that sets a state that causes another set to update.
What if we created a useEffect
that updates the timeInterval
, but that doesn't depend on timeInterval
? This is possible with the functional version of the setState
function:
useEffect(() => {
if (animationEnabled) { // this might read clearer than playButtonDisabled
const monthlyInterval = setInterval(
() => setTimeValue(currentTimeValue => currentTimeValue + 1),
25
);
return () => { clearInterval(monthlyInterval); };
},
}, [animationEnabled]);
and then created a separate useEffect
(s) for stuff that should happen each time the interval updates
useEffect(() => {
if (timeValue == PROGRESS_FINAL_MONTH) {
setAnimationEnabled(false); // this will cause the above useEffect to clean up and clear the interval
}
...other stuff that changes with each interval change (or in separate use effects)
}, [timeValue]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome suggestion, thank you! Just implemented your refactor as part of 8112efc and popped in a console.log to verify we only have one setInterval
fired and its appropriately cancelled for each map play. 👍
<Box | ||
w='70px' | ||
h='40px' | ||
bg='#94A4DF' | ||
textAlign={'center'} | ||
color={'white'} | ||
fontSize={'sm'} | ||
pt='8px' | ||
> | ||
≥1% BIL | ||
</Box> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting this to a named component that can be reused to create the other blocks.
def90a4
to
aeb6ae4
Compare
aeb6ae4
to
bcc6e30
Compare
Thanks all for the review! I implemented these fixes and this is set for another look.
Great idea! The above is captured this in issue #44 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for updating the useEffects.
useEffect(() => { | ||
if(animationEnabled){ | ||
const monthlyInterval = setInterval(() => { | ||
setTimeValue(currentTimeValue => Math.round((currentTimeValue + 0.1)*10)/10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took me a minute to figure out why this math is necessary. Then I realized:
.1 + .1 + .1 === .30000000000000004 // true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bah I know, adding in those smaller increments really help to break down the progress frames and flow like an animation but this line still feels unnecessarily hacky.
Overview
Adds an "animated" progress bar and play button to view spending over time.
Closes #24
Demo
Screen.Recording.2023-02-28.at.3.04.15.PM.mov
Notes
setInterval
callback, that way there is one constant in state that updates both the progress bar UI and layer colors for that month value so they're as in sync as possible. That value is broken down into 0.2 increments to allow for a more smooth progress bar transition, and the layers will only update colors once the value arrives at a value that correlates with the start of a new month (e.g. whole number)Testing Instructions
scripts/server
Checklist
fixup!
commits have been squashedCHANGELOG.md
updated with summary of features or fixes, following Keep a Changelog guidelinesREADME.md
updated if necessary to reflect the changes