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

Added option to disable map styles #969

Closed
wants to merge 2 commits into from

Conversation

WGrobler
Copy link
Contributor

I want to disable certain map styles server wide, therefore I included attributes for each provider as well as an option to construct the styles array based on the attributes. To limit the attributes, I created it on provider level; we can change it to individual level, to allow disabling of individual styles.

I would like to hear thoughts and possible improvements

Copy link
Member

@tananaev tananaev left a comment

Choose a reason for hiding this comment

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

I don't think it makes sense to make an attribute for each map. I think one attribute with a list should do it. And also it should probably be server-only attribute.

@WGrobler
Copy link
Contributor Author

When you say "one attribute with a list", are you thinking of a string attribute with comma separated values or a list of maps linked to a bool attribute (like I did here)?

You are right that the attributes should be server-only, I'm still familiarising myself with the entire codebase, how do I set it server only? is there a useAttributePreference for server only or should I do something similar to useFeatures.js#L3

@tananaev
Copy link
Member

Comma-separated list of disabled maps.. or maybe enabled.

You don't need to use useAttributePreference. You can just get server from redux state directly.

@WGrobler
Copy link
Contributor Author

I was also initially considering a list, thought bools would be easier since I can include it in the common attributes so that it can autofill, but since this would be a server setup it does not require autofill for each style. I agree with you and will change it to a list instead.

I think the list needs to be a disabled list. Most instances will enable everything by default but then this is the option to disable. Otherwise we complicate existing instances.

Looking at the code for useAttributePreference I can see that it forces the server setting, therefore is it required to make this server only? It might not be used for users, but does it matter if its possible for them to set it since they can't override server?

@WGrobler
Copy link
Contributor Author

Is there an common/attributes for server only attributes or must it be included in attributes/useCommonUserAttributes.js?

@tananaev
Copy link
Member

therefore is it required to make this server only?

Yes. We don't want to pollute user configuration. And also "force server settings" is a flag that's disabled by default.

Common attributes are common for server and user. Instead you should create a separate one for server. Similar to how we have useUserAttributes.

@WGrobler WGrobler mentioned this pull request Jun 21, 2022
@WGrobler
Copy link
Contributor Author

Made changes as discussed and create a new PR #970

@WGrobler WGrobler closed this Jun 21, 2022
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