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

feature: add thematicStyling to sharemap #1962

Merged
merged 7 commits into from
Feb 20, 2024
Merged

Conversation

dahlalex
Copy link
Contributor

@dahlalex dahlalex commented Feb 9, 2024

@dahlalex dahlalex changed the title Added activeThemes Sharemap ThematicStyling: Added activeThemes Feb 9, 2024
@dahlalex dahlalex self-assigned this Feb 9, 2024
@dahlalex dahlalex linked an issue Feb 9, 2024 that may be closed by this pull request
@dahlalex dahlalex changed the title Sharemap ThematicStyling: Added activeThemes feature: add thematicstyling to sharemap Feb 9, 2024
@dahlalex dahlalex changed the title feature: add thematicstyling to sharemap feature: add thematicStyling to sharemap Feb 9, 2024
@johnnyblasta
Copy link
Collaborator

Tested and works fine for GeoJSON and WFS (I assume, from the code, didn't test that), but it don't work for WMS.

@johnnyblasta
Copy link
Collaborator

One thought:

  • The config part would be more simple if the label was used instead of constructing an array with zeros and ones.

I've tried to use this logic:
style[0].thematic[index].visible = false;

It's not working. Any ideas?
@dahlalex
Copy link
Contributor Author

I've made som changes to work also for WMS and found an issue that

origo.api().getStyles()[styleName][0].thematic[index].visible = false

has no effect.

It seems that this issue should be solved even if the label is used (if i understand correctly).

@johnnyblasta
Copy link
Collaborator

I've made som changes to work also for WMS and found an issue that

origo.api().getStyles()[styleName][0].thematic[index].visible = false

has no effect.

It seems that this issue should be solved even if the label is used (if i understand correctly).

ThematicStyling for WMS works by filtering the request, so I guess a new request has to be triggered.

@dahlalex
Copy link
Contributor Author

Thanks for the helpful feedback! I've implemented your suggestion regarding the label, and it's working successfully even for WMS. You're absolutely right - using a label simplifies the configuration and eliminates the dependency on the specific order of zeros and ones in the array, which was definitely a downside. This approach makes the code more flexible and easier to maintain in the long run.

@Grammostola
Copy link
Contributor

Grammostola commented Feb 16, 2024

It appears to work for WMS now, nice : ) However the link uses rule titles rather than rule names (whereas getLegendGraphic for instance is forced to use rule names). Rule names are ids and titles are for humans so I think it would be better if it the link was saved with rule names.

Similarly for vector styles I thought an optional id prop for themes in a thematic style was a good idea since the same thing applies: labels are for human reading and can be long and colorful.

@dahlalex
Copy link
Contributor Author

Excellent point! 😊 You're absolutely right about using rule names instead of titles for the WMS and theme IDs for vector styles. I've already made some changes and ensured both elements use IDs and names first, followed by labels where applicable. This creates a clearer distinction between human-readable labels and machine-readable identifiers.

@dahlalex
Copy link
Contributor Author

In the shared link, I'm still unsure about the best separator to use between themes. I chose the tilde (~) because it's less common and therefore less likely to cause conflicts. What do you think? Do you have another suggestion that might be even better?

@Grammostola
Copy link
Contributor

Cool, looks good now for WMS. For vector layers no themes are active when opening the shared map. I don't have an opinion on the separator, tilde is less common and this would likely do well described in the documentation somewhere too.

@dahlalex
Copy link
Contributor Author

dahlalex commented Feb 16, 2024

It works for me. Can you share the link you generate after clicking the "Share Map" button?

The issue might be due to vector styles lacking both IDs and labels. Adding either IDs or labels to these styles could potentially resolve the problem.

@Grammostola
Copy link
Contributor

I used your example theme style for the origo cities layer and added an id prop to both. http://localhost:9966/#layers=osm/v/1/s/0/o/100,origo-mask/v/1/s/0/o/25,origo-cities/v/1/s/0/o/100/th/2&center=1810000,8390000&zoom=5.7&legend=expanded&map=index

...And the problem seems to be Number vs string. I gave the id prop a number value. Maybe both could be acceptable?

@dahlalex
Copy link
Contributor Author

You're right! The number value for the id prop was causing the issue. Thanks for identifying this crucial detail. Converting it to a string seems to have corrected the problem.

Copy link
Contributor

@Grammostola Grammostola left a comment

Choose a reason for hiding this comment

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

Looks good to me. Tested with geojson, wfs and wms layers.

@dahlalex dahlalex merged commit 3096f03 into master Feb 20, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Hallstahammar
Development

Successfully merging this pull request may close these issues.

Sharemap doesn't work with thematicStyling
3 participants