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

Vector layer can hide layer symbology when conditional styling used #222

Merged
merged 3 commits into from
Mar 31, 2023

Conversation

michaelpnelson
Copy link
Contributor

In some uses of conditional styling, styling is applied to feature values in a way that each feature is included and matches one style, similar to a pie chart. In such situations, including a style for the entire layer in a legend is unnecessary.

This adds a "hideLayer" property to legend configuration in vector layers and to legend display HTML. When added to a vector/GeoJSON layer's legend configuration and set to true, the symbology for the layer (but not its conditional styling) is omitted in the legends.

In some uses of conditional styling, styling is applied to feature values in a way that each feature is included and matches one style, similar to a pie chart. In such situations, including a style for the entire layer in a legend is unnecessary.

This adds a "hideLayer" property to legend configuration in vector layers and to legend display HTML. When added to a vector/GeoJSON layer's legend configuration and set to true, the symbology for the layer (but not its conditional styling) is omitted in the legends.
@michaelpnelson michaelpnelson requested a review from dgboss March 27, 2023 20:21
@michaelpnelson michaelpnelson self-assigned this Mar 27, 2023
@dgboss
Copy link
Contributor

dgboss commented Mar 29, 2023

Do you have an example of this behavior or a description on how to reproduce?

@michaelpnelson
Copy link
Contributor Author

Do you have an example of this behavior or a description on how to reproduce?

I think it's easiest to show the difference :)

Our use case is for the CleanBC app. The Ministry EVCS layer uses conditional styling to set the colour of a feature based on the stall type. Each feature is one of the two stall types we conditionally style for, but we still show a style for the layer itself, which seems redundant:

Screenshot 2023-03-29 at 8 33 41 AM

With this change, and a change to CleanBC to add "hideLayer": true to the layer's legend config, we modify the legend display to make more sense:

Screenshot 2023-03-29 at 8 36 05 AM

@dgboss
Copy link
Contributor

dgboss commented Mar 29, 2023

Thanks for the explanation and pictures! My recommendation would be to never show the swatch for the layer when conditional styling is present. If you want to be able to show the default style for the layer, it is common practice to display a 'default' entry below the entries for conditional styles. You could have config to show/hide this default symbol and a 'label' property or something like that so you can configure what text to display instead of 'default'. This is a more typical approach for web maps.
image

Also, the layer name needs some whitespace to the left.

@michaelpnelson
Copy link
Contributor Author

Thanks for the explanation and pictures! My recommendation would be to never show the swatch for the layer when conditional styling is present. If you want to be able to show the default style for the layer, it is common practice to display a 'default' entry below the entries for conditional styles. You could have config to show/hide this default symbol and a 'label' property or something like that so you can configure what text to display instead of 'default'. This is a more typical approach for web maps. image

Also, the layer name needs some whitespace to the left.

Thanks for this suggestion. I like the idea of making things more consistent by always omitting legend styling for a layer's name when the layer has conditional styling.

Do you happen to have a link or two handy where this style of legend appears? I don't have much reference for this style.

I think I'd like to get more feedback from others (especially UX advice from Jeffrey) about this approach also. I agree that it would be good to be able to configure "default" labeling.

@dgboss
Copy link
Contributor

dgboss commented Mar 29, 2023

The following link has a legend with an 'Other' entry which is analogous to the 'default'.
https://developers.arcgis.com/javascript/latest/sample-code/visualization-location-types/

* Documentation is updated for vector/GeoJSON layer
* Legend swatches are now always omitted when conditional styling is used
* Property "includeOtherLegendWithDefaultStyling" is added to legend configuration in the vector layer. When set to true, an "Other" legend is added for the layer and the swatch is styled with the default styling.
* Legend tool CSS is updated to add padding
@michaelpnelson
Copy link
Contributor Author

Here is what legends look like after changes in the latest commit. The new "includeOtherLegendWithDefaultStyling" legend property has been set to true for layer "Electric Charging Stations in BC".

Screenshot 2023-03-30 at 10 20 52 AM

@@ -24,7 +24,8 @@ include.module( 'layer.layer-vector-js', [ 'layer.layer-js' ], function () {
const legendData = [];
legendData.push({
title: self.config.legend && self.config.legend.title || self.config.title,
style: self.config.style
style: self.config.style,
hasConditionalStyling: self.config.conditionalStyles || false
Copy link
Contributor

Choose a reason for hiding this comment

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

While JavaScript allows the assignment of an object or a boolean to the same variable, it is not good practice. I'd suggest assigning true/false or assigning the object/undefined to hasConditionalStyling.

Copy link
Contributor

Choose a reason for hiding this comment

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

My choice would be true/false as you never actually access the object properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm checking for the existence of the "conditionalStyles" property in config, but yes, it is kind of dumb to actually assign it when it's present :) . I'll update this to do a better check of existence.


Set the shape of the legend swatch. "line" is the default.

`"includeOtherLegendWithDefaultStyling": Boolean`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On further thought, I think I will reverse this to something like "hideOtherLegendWithDefaultStyling". I think we should include the default styling as a ... default :) . I think most uses of conditional styling will want to include the default, and I want to favour inclusion.

@michaelpnelson michaelpnelson merged commit c179bd1 into master Mar 31, 2023
@michaelpnelson michaelpnelson deleted the hide_layer_legend branch March 31, 2023 16:04
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