Open
Conversation
henhuy
requested changes
May 24, 2023
Contributor
henhuy
left a comment
henhuy
left a comment
There was a problem hiding this comment.
I would recommend to move MapLayerModel to mapengine models.py.
| if settings.MAP_ENGINE_MAPLAYER_MODEL: | ||
| # get MapLayerModel from settings | ||
| try: | ||
| MapLayerModel = apps.get_model(app_label='map', model_name=settings.MAP_ENGINE_MAPLAYER_MODEL) |
Contributor
There was a problem hiding this comment.
I thought the MapLayerModel would be integrated in mapengine's models.py ?
This would make model available in all projects which use mapengine automatically.
Also this would remove need for app.get_model and currently hardcoded "map" namespace.
|
|
||
| # get choropleth fields | ||
| try: | ||
| model_field = MapLayerModel._meta.get_field("choropleth_field") |
Contributor
There was a problem hiding this comment.
This would also be obsolete, as we make sure field exists by providing model from within mapengine
Contributor
|
Hi @finnus , when you create the model please add a placeholder for unit in case some models dont have one |
Merged
henhuy
force-pushed
the
optional_map_layer_model
branch
from
56b7cdf to
d6dd963
Compare
Open
…ptional_map_layer_model # Conflicts: # django_mapengine/static/django_mapengine/js/legend.js
initStore has been overridden by digiplan
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The idea behind the PR is to put more configuration of the map layers into django models.
That would have some advantages (more flexible, no need to change code when changing minor things or even adding/removing layers/choropleths/popups).
This is a first guess into that direction, with a few downsides:
As this approach looks still promising to me, I added the respective code. If no MapLayerModel is defined in the settings, it should work as before.