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

Disable reanimate options change #99

Closed

Conversation

wecraw
Copy link
Contributor

@wecraw wecraw commented Jun 10, 2024

I'm submitting a new:

[ ] Bug Fix
[ x ] Feature
[ ] Other (Refactoring, Added tests, Documentation, ...)

Description

Adds support for a new option disableReanimateOnOptionsChange to prevent a component controlled by countUp from reanimating when options are changed.

When this option is set to true, the component will only reanimate when endVal is changed, or explicitly triggered by other means.

Does this PR introduce a breaking change?

[ ] Yes
[ x ] No

How to test:

  1. Add the new optional property to countUp.d.ts
  2. Under useOptions() in app.component.ts, add the additional option disableReanimateOnOptionsChange: true
  3. Run the app, observe that changing the options does not trigger a reanimation, but the new options are still reflected when the endVal is changed and the reanimation is triggered.

@wecraw wecraw marked this pull request as draft June 10, 2024 20:36
@wecraw
Copy link
Contributor Author

wecraw commented Jun 10, 2024

@inorganik Hey Jamie, first time making an open source contribution so forgive the noob question: This PR depends on changes to countUp.d.ts, which isn't tracked here. Do I need to make a PR against the core countUp library first?

Thanks in advance!

@inorganik
Copy link
Owner

Hey Will, so is there a bug where it re-animates when options besides endVal are changed, and you are trying to fix it here?

@wecraw
Copy link
Contributor Author

wecraw commented Jun 10, 2024

Hey Will, so is there a bug where it re-animates when options besides endVal are changed, and you are trying to fix it here?

It was unclear to me if this was a bug or a feature.

My use case is this: On first load, I'd like countUp to run with a certain duration. On subsequent animations (i.e. when the countUp value changes), I want the animation to run with a shorter duration. However, changing the options input immediately triggers a reanimation even if the underlying count hasn't changed.

I'm sure there are some workarounds for this on the app side, but having official support would be better.

@inorganik
Copy link
Owner

Couldn't you just update the options and the endVal at the same time? The reason we don't already have this option is because you would only ever need to update the options if you plan to animate again. And if you are animating again, then just set the options when you want it to animate. You can set the endVal inside the options object.

Regarding your first question about the dependency on CountUp, no you don't need to PR there (I maintain both repos). But instead of going straight to making a PR, always create an issue first to validate that you have an issue, then usually the repo maintainers will tell you if it needs a PR or not. In this case I don't think you do.

@wecraw
Copy link
Contributor Author

wecraw commented Jun 11, 2024

Thanks for the suggestion, I was able to get it to work this way. One oddity, I have to manually trigger the change detection to get it to register the new settings:

  public countUpOptions = {duration: 1.5}
  public showStep2: boolean = false;

  updateTotals(){
    if (this.showStep2) {
      this.countUpOptions = {duration: 0.2}
      this.changeDetectorRef.detectChanges();
    } 
    
    this.totalWattHours = this.calculationUtils.totalWattHours(this.build)
    this.peakWattage = this.calculationUtils.peakWattage(this.build)

    this.showStep2 = true
  }
<span [countUp]="peakWattage" [options]="countUpOptions" class="number">0</span>

Without the detectChanges() it will always just use the original {duration: 1.5}

At any rate, I will close this if this is the preferred way to use the lib. Thanks for the thoughtful replies and awesome library!

@inorganik
Copy link
Owner

I looked into your issue and there is indeed a bug - you can't set options and endVal at the same time. I tested it in the test app of this repo and in a new angular 18 app. Also, I was wrong about being able to set endVal in the options (sorry).

The reason is because in the directive, there's an if/else statement in ngOnChanges, making it so it's an either/or situation.

I can make a fix for this, but it probably won't get merged till next week. I would encourage you to take a crack at it if you want but really I should update angular and have it depend on the latest countUp as well. Thanks for finding this issue!

@wecraw
Copy link
Contributor Author

wecraw commented Jun 11, 2024

Thanks for looking into this, glad to take a stab at a fix!

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.

2 participants