-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Disable styles #970
base: master
Are you sure you want to change the base?
Disable styles #970
Conversation
modern/src/map/core/useMapStyles.js
Outdated
]; | ||
}; | ||
|
||
Object.keys(Styles).forEach((key) => { |
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 think the right think is to keep the array as is and just filter it. Otherwise the order is not guaranteed and you kind of have duplication of the ids.
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.
Are you suggesting that we use an array for Styles
(like it currently is in master) and delete styles from it if found in the disabledStyles
list? Or do you suggest that we still loop over Styles
and create activeStyles
, but only changing Styles
back to a list instead of an dict?
I thought about looping though the disabledStyles
set, looking for the id in Styles
and removing it if found. The problem is that it would require a search though the entire Styles
array for every entry in disabledStyles
. Since disabledStyles
will always be smaller than Styles
, I decided that it would be better to perform the "has key" check on the disabledStyles
set.
Changing Styles
back to an array and still using the same logic where its copied over to activeStyles
if not in disabledStyles
, should solve your order and id duplication concerns.
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.
Changing
Styles
back to an array and still using the same logic where its copied over toactiveStyles
if not indisabledStyles
, should solve your order and id duplication concerns.
I made this change with d17e1b3
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.
Added some small comments, but overall looks pretty good.
|
||
return [ | ||
const Styles = [ |
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.
variables shouldn't start with a capital letter. it should be lower case
@@ -30,8 +30,10 @@ export default () => { | |||
const hereKey = useAttributePreference('hereKey'); | |||
const mapboxAccessToken = useAttributePreference('mapboxAccessToken'); | |||
const customMapUrl = useSelector((state) => state.session.server?.mapUrl); | |||
const disabledStyles = new Set((useSelector((state) => state.session.server.attributes?.disableMapLayers) || '').split(',') || []); |
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 would split it into two. First get disableMapLayers
using selector and then a separate line to convert it into a set.
Also I think || []
should be unnecessary.
for (let i = 0; i < Styles.length; i += 1) { | ||
if (!disabledStyles.has(Styles[i].id)) { | ||
activeStyles.push(Styles[i]); | ||
} | ||
} | ||
return activeStyles; |
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.
Just use styles.filter(...)
instead of doing this loop. And no need for a separate variable in that case.
@@ -58,6 +59,7 @@ const ServerPage = () => { | |||
const t = useTranslation(); | |||
|
|||
const commonUserAttributes = useCommonUserAttributes(t); | |||
const commonServerAttributes = useServerAttributes(t); |
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.
You forgot to rename the variable.
Any plans to update this or should we close it for now? |
Update to PR #969
disableStyles
to useCommonServerAttributesattributeUiDisableStylesAttributes