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

feat: add setOptions and setAxis method #198

Merged
merged 4 commits into from
Aug 18, 2022

Conversation

malangfox
Copy link
Contributor

@malangfox malangfox commented Aug 11, 2022

Details

Among the cases of using Axes, there is a case of directly changing the options of an Axes instance such like below.

...
    const axes = this._axes!;
    const axis = axes.axis[AXES.POSITION_KEY];
    axis.circular = [controlParams.circular, controlParams.circular];
    axis.range = [controlParams.range.min, controlParams.range.max];
    axis.bounce = parseBounce(flicking.bounce, camera.size);
...

This add setOptions and setAxis methods to change the options related to the Axes instance to simplify use cases like this.

Changing Options

AxesOption

Changing easing, interruptable can be applied to an animation currently in progress.
Changing maximumDuration, minimumDuration, deceleration applies when calculating to play the animation on next time. If you want to change the remaining time of an animation that is playing too, you may use updateAnimation method.
Changing round, nested can applied after inputs that made after changing these options.

AxisOption

Changing range, bounce, circular can applied after inputs that made after changing these options.
If the current position is outside the range set in the new option, it does not move the coordinates within the range directly since it can trigger change event.
However, when next user input occur, new range will be applied in the process of calculating the new position. Triggering change event to position within range.

Copy link
Member

@daybrush daybrush left a comment

Choose a reason for hiding this comment

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

LGTM

* ```
*/
public setOptions(options: AxesOption) {
Object.assign(this.options, options);
Copy link
Member

Choose a reason for hiding this comment

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

I think this will remain in the build, please check whether it does.
As Object.assign isn't supported in IE

Copy link
Member

Choose a reason for hiding this comment

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

Also, aren't there any internal updates required for this change?
Check this example of updating options in View360

Like, nested may require some immediate update of event handlers.

Copy link
Contributor Author

@malangfox malangfox Aug 16, 2022

Choose a reason for hiding this comment

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

It looks like this changes should avoid using Object.assign for browser compatibility. Confirmed that it remains after the build.
Also changing the nested option will not require any changes to the event handler.
We are using the following statement to add property to native event.

    if (!this.options.nested || !this._isEndofAxis(offset, depaPos, destPos)) {
      nativeEvent.__childrenAxesAlreadyChanged = true;
    }

Since the option is checked when each change is made, the changed option will be applied from the next input without changing any event handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are parts that need immediate change, it may be current animation being played that duration needs to be changed by minimum/maximum duration option.
Also it will be necessary to discuss about action when the current range is outside of the new range.

if (!this._axis[key]) {
throw new Error(`Axis ${key} does not exist in Axes instance`);
}
Object.assign(this._axis[key], axis[key]);
Copy link
Member

Choose a reason for hiding this comment

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

Same here, you can do it rather like this, and the typescript compiler(+ bundler) will do the work

this._axis[key] = {
  ...this._axis[key],
  ...axis[key]
}

Copy link
Member

@WoodNeck WoodNeck left a comment

Choose a reason for hiding this comment

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

LGTM

@malangfox malangfox merged commit 1bcb607 into naver:master Aug 18, 2022
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.

None yet

3 participants