-
Notifications
You must be signed in to change notification settings - Fork 45
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
Feature/764 background layer info #808
Conversation
# Conflicts: # new-client/src/plugins/LayerSwitcher/components/BackgroundSwitcher.js
# Conflicts: # new-client/src/plugins/LayerSwitcher/components/LayerItem.js
new-client/src/plugins/LayerSwitcher/components/LayerSettings.js
Outdated
Show resolved
Hide resolved
new-client/src/plugins/LayerSwitcher/components/BackgroundSwitcher.js
Outdated
Show resolved
Hide resolved
new-client/src/plugins/LayerSwitcher/components/BackgroundSwitcher.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check and solve all basic compilation warnings prior PR.
Also, the layout looks bad compared to how it was before, ensure that you don't modify the left padding this much.
I'm closing this PR for now, please verify the code (perhaps ask @jesade-vbg to do some checks too) before you open the PR again.
…ckground-layer-info # Conflicts: # new-client/package-lock.json
I have updated the code and gotten an additional review from Jesper. The code has been merged with the latest from develop as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can not approve a PR where most of the changes come from different Prettier settings.
Please take a look at the PR you're submitting. You can clearly see that something's wrong looking only at the number of changed files (33!).
I think that the formated lines in the .md files and new-admin/src/views/informativeeditor.jsx are correct with the changes in this PR so I left it. It is starting to become late here so I will run some tests tomorrow to confirm everything is working after the formatting updates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this PR contain changes to CONTRIBUTING.md and README.md?
When I reverted the formatting I noticed that the changes to CONTRIBUTING.md and README.md was probably correct, i.e. correctly formatted now and therefore left it. I can revert it if you want me to. Same with new-admin/src/views/informativeeditor.jsx. |
Absolutely, your PR should only contain actual changes related to this issue - not unresolved conflicts or reverts to previous versions. |
Reverted the prettier modifications for the other files |
LGTM |
Pull request for issue #764
I have modified so Backgroundswitcher uses LayerItem to display its layers. This introduced errors since the default layers (vit, svart and OSM) does not have enough metadata or even OL layers. I added extra metadata for these layers in Backgroundswitcher to ensure they do not cause a crash in LayerItem or LayerSettings.
We can now configure the layerinformation in admin in the same way as for normal layers and it is displayed for the background layers in the same manner so we do not have to duplicate code.