Skip to content

Conversation

fschmenger
Copy link
Collaborator

This PR updates the ViewAnimation utility to improve encapsulation by changing its constructor signature. Instead of receiving the entire application context object, it now accepts only the viewAnimation sub-property. This aligns with a pattern already used elsewhere in the codebase.

To avoid introducing a breaking change, the constructor still accepts the full application context for now. However, passing the full context will trigger a deprecation warning.

…construction argument. For backward compatibility allow the full appConfig as parameter but prompt a warning.
@fschmenger fschmenger force-pushed the view_animation_config branch from 6299c07 to 4fff2a2 Compare September 4, 2025 16:10
Copy link
Collaborator

@sronveaux sronveaux left a comment

Choose a reason for hiding this comment

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

This one is a nice refactoring, this makes more sense than passing the whole configuration object for sure !

I just suggested to change a parameter definition in a comment but I must admit I never verified in the rest of the codebase whether this change should be applied elsewhere or not so it's really not a problem if kept as is.

Feel free to merge this one and thanks again for the contribution !

Copy link
Collaborator

@chrismayer chrismayer left a comment

Choose a reason for hiding this comment

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

Very nice, thanks for this 👍

…nUtil object with the full application context.
@fschmenger
Copy link
Collaborator Author

Thanks for the reviews!

As suggested by @chrismayer I removed the backward compatibility option.

@sronveaux: A quick search for the usage of optional parameters yielded several modules, e.g. factory/Layer.js, util/Layer.js LayerLegends.js and others. To keep things consistent I propose to introduce the "bracket syntax" in a separate PR.

Let me know if this is fine for you, then I will merge.

@fschmenger fschmenger merged commit dbef4d5 into wegue-oss:master Sep 25, 2025
1 check passed
@sronveaux
Copy link
Collaborator

Hi @fschmenger and thanks again for this !

You are totally right concerning optional parameters in JSDoc comments and did well... I just opened issue #448 to keep this in mind in case someone has some time to spend on this one day...

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.

3 participants