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

Remove assignement of all config properties in base Layer class #2368

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

Desplandis
Copy link
Contributor

@Desplandis Desplandis commented Jul 22, 2024

Description

This PR aims to remove the call to Object.assign(this, config) in the Layer class and move-up all properties initialisation in their respective classes. The goal of this refactoring is triple: ease the understanding of itowns logic (side-effects, initialisation, etc...), type-checking and a first step to clean-up configurations and layers.

Closes #2368

@Desplandis Desplandis force-pushed the remove/layer-assign branch 6 times, most recently from ca64d3d to 6ba93e7 Compare July 22, 2024 16:41
@Desplandis Desplandis self-assigned this Jul 24, 2024
@jailln jailln self-requested a review September 19, 2024 15:48
@jailln
Copy link
Contributor

jailln commented Sep 19, 2024

Nice! Can you rebase and fix the failing test please ? Then I can review it

@Desplandis
Copy link
Contributor Author

@jailln Rebased and ready to go!

@jailln jailln self-assigned this Nov 6, 2024
Copy link
Contributor

@jailln jailln left a comment

Choose a reason for hiding this comment

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

Thanks :) it's way clearer this way

src/Layer/ElevationLayer.js Show resolved Hide resolved
src/Layer/FeatureGeometryLayer.js Show resolved Hide resolved
src/Layer/Layer.js Show resolved Hide resolved
@Desplandis
Copy link
Contributor Author

Rebased, RTM.

@Desplandis Desplandis merged commit cf41e8d into iTowns:master Nov 27, 2024
9 checks passed
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