-
Notifications
You must be signed in to change notification settings - Fork 17
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
Comparative Formula Widget #504
Conversation
Pull Request Test Coverage Report for Build 3346188627
💛 - Coveralls |
- encapsulate animation effects in widgets - wrap around `animateValue` in utils - provide JSDoc documentation - provide TS typings
const requestAnimationFrameRef = useRef(-1); | ||
|
||
// if we want to run the animation on mount, we set the start value as 0 and animate to the start value | ||
const [animatedValue, setAnimatedValue] = useState(() => animateOnMount ? 0 : value); |
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.
I like the idea of a hook to encapsulate animation. I think it's a good addition!
Just one comment, we're assuming that values are positive, and we should always animate from 0. But what about adding an extra param to indicate initialValue?: number
? (with 0 as default)?
useEffect(() => { | ||
if (!disabled) { | ||
animateValue({ | ||
start: animatedValue || 0, |
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.
to be used also here... initialValue
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.
Here also?
Wouldn't animatedValue
get assigned the value of initialValue
in the useState
call a few lines above?
I was setting this || 0
here in the cases where value
is a "falsy" value, but as this check is also performed in <AnimatedNumber />
we can remove it from here if you see it ok
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.
Then I'm ok
packages/react-ui/src/types.d.ts
Outdated
|
||
export type AnimationOptions = { | ||
duration?: number; | ||
animateOnMount?: boolean; |
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.
to add here the initialValue?: number
?
…use-animated-number
…comparative-formula-widget
Description
This PR implements part of the work proposed in RFC #479
Changes added in this PR include:
ComparativeFormulaWidgetUI
in thewidgets
folderdependencies
ofpackages/react-ui/package.json
used for animationsAnimatedNumber
in thecustom-components
folder. This component uses theuse-animated-number
npm package and abstracts away the logic for animating a changing number that is required for both formula widget and category widget, removing almost entirely the need foruseEffect
in UI components.AnimatedNumber
andComparativeFormulaWidgetUI
AnimatedNumber
andComparativeFormulaWidgetUI
AnimatedNumber
andComparativeFormulaWidgetUI
ComparativeFormulaWidgetUI
where all prop combinations can be tested and tweaked. A screenshot of this storybook is added as visual feedback.Type of change
Acceptance
How to validate the feature:
yarn storybook:start
in thereact-ui
folder.ComparativeFormulaWidgetUI
in your sample project, by importingComparativeFormulaWidgetUI
from@carto/react-ui
. You can use this branch ofcarto-react-template
to skip this step if you want, as a test screen is already setup there. Bear in mind that if this feature is not published to npm you will have to link your local version of@carto/react-*
packages withyarn link
.Basic checklist