Skip to content
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

Merged
merged 19 commits into from
Oct 28, 2022
Merged

Conversation

juandjara
Copy link
Contributor

@juandjara juandjara commented Oct 24, 2022

Description

This PR implements part of the work proposed in RFC #479

Changes added in this PR include:

  • A new widget called ComparativeFormulaWidgetUI in the widgets folder
  • A small npm package added to dependencies of packages/react-ui/package.json used for animations
  • A new component called AnimatedNumber in the custom-components folder. This component uses the use-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 for useEffect in UI components.
  • Typescript typings for both AnimatedNumber and ComparativeFormulaWidgetUI
  • JSDoc typing comments for both AnimatedNumber and ComparativeFormulaWidgetUI
  • Prop-types validation for both AnimatedNumber and ComparativeFormulaWidgetUI
  • A basic storybook story for ComparativeFormulaWidgetUI where all prop combinations can be tested and tweaked. A screenshot of this storybook is added as visual feedback.

Captura de pantalla 2022-10-24 a las 14 20 04

Type of change

  • Feature

Acceptance

How to validate the feature:

  1. Open the local storybook running yarn storybook:start in the react-ui folder.
  2. Pick a set of properties to use for testing by tweaking the storybook configuration.
  3. Use this set of properties to render a ComparativeFormulaWidgetUI in your sample project, by importing ComparativeFormulaWidgetUI from @carto/react-ui. You can use this branch of carto-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 with yarn link.
  4. Check that the rendered version of the widget in your sample project looks the same as it looks in the storybook.

Basic checklist

  • Good PR name
  • Shortcut link
  • Changelog entry
  • Just one issue per PR
  • GitHub labels
  • Proper status & reviewers
  • Tests
  • Documentation

@coveralls
Copy link
Collaborator

coveralls commented Oct 24, 2022

Pull Request Test Coverage Report for Build 3346188627

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 71.74%

Totals Coverage Status
Change from base Build 3339127632: 0.0%
Covered Lines: 1703
Relevant Lines: 2219

💛 - 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);
Copy link
Contributor

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,
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I'm ok


export type AnimationOptions = {
duration?: number;
animateOnMount?: boolean;
Copy link
Contributor

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?

@VictorVelarde VictorVelarde merged commit 0a26ca2 into master Oct 28, 2022
@VictorVelarde VictorVelarde deleted the feature/comparative-formula-widget branch October 28, 2022 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants