Disable styles #970
Disable styles #970
Conversation
modern/src/map/core/useMapStyles.js
Outdated
| ]; | ||
| }; | ||
|
|
||
| Object.keys(Styles).forEach((key) => { |
There was a problem hiding this comment.
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.
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.
Changing
Stylesback to an array and still using the same logic where its copied over toactiveStylesif not indisabledStyles, should solve your order and id duplication concerns.
I made this change with d17e1b3
tananaev
left a comment
There was a problem hiding this comment.
Added some small comments, but overall looks pretty good.
| const activeStyles = []; | ||
|
|
||
| return [ | ||
| const Styles = [ |
There was a problem hiding this comment.
variables shouldn't start with a capital letter. it should be lower case
| 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.
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.
Just use styles.filter(...) instead of doing this loop. And no need for a separate variable in that case.
| const t = useTranslation(); | ||
|
|
||
| const commonUserAttributes = useCommonUserAttributes(t); | ||
| const commonServerAttributes = useServerAttributes(t); |
There was a problem hiding this comment.
You forgot to rename the variable.
|
Any plans to update this or should we close it for now? |
Update to PR #969
disableStylesto useCommonServerAttributesattributeUiDisableStylesAttributes