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

Remove grid format, mod, and legend; optional layer qid; vector format viewport; featureFormats util & theme.field_stats #871

Merged
merged 7 commits into from Aug 21, 2023

Conversation

dbauszus-glx
Copy link
Member

@dbauszus-glx dbauszus-glx commented Aug 14, 2023

The grid format has been removed as it is currently bugged. #873

In order to replace the grid format the vector format must be configurable to reload the data within the viewport when the viewport changes. Vector layer will reload the data from source with a viewport if the viewport: true flag is set on the layer. The layer.reload() will be triggered from the mapview changeEnd event.

Vector layer will use layer.tableCurrent() instead of layer.table and not load data from source if there is no current table.

Vector feature properties should be spread on the vector feature and not be assigned as feature.properties.properties.

The grid format had a dynamic theme.

Dynamic themes require stats for the field used in theme. These stats can only be calculated from a series of field values which will be populated when the features are processed. The theme.updated flag will be set allowing the theme method to only process the required field statistics once prior to deleting the flag. #306

The featureFormats have been removed from the vector module and added as mapp.utils.

The optional layer qID bug #874 has been addressed in this issue.

@dbauszus-glx dbauszus-glx changed the title optional layer qid, vector format viewport Remove grid format, mod, and legend; optional layer qid; vector format viewport; featureFormats util & theme.field_stats Aug 15, 2023
@dbauszus-glx dbauszus-glx linked an issue Aug 15, 2023 that may be closed by this pull request
@dbauszus-glx dbauszus-glx marked this pull request as ready for review August 15, 2023 14:39
@RobAndrewHurst
Copy link
Contributor

The styling on WKT layers doesn't seem to be applying.

image

@dbauszus-glx
Copy link
Member Author

@RobAndrewHurst Styling should be fixed in 56e27d7

The properties object must not be spread when being assigned to the feature.

@RobAndrewHurst
Copy link
Contributor

@dbauszus-glx I noticed a bit of code duplication in the featureFormats.mjs module.

There was an if statement

if (layer.style?.theme?.field_stats) {
        layer.style.theme.updated = true;
        layer.style.theme.field_stats[layer.style.theme.field] = {
            values: []
        };
    }

which I then put into a function initializeFieldStats

which is now referenced in both the geojson and wkt functions.

Let me know if you are happy with that!

Copy link
Contributor

@RobAndrewHurst RobAndrewHurst left a comment

Choose a reason for hiding this comment

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

Code Review for PR #871

😃 I've reviewed the PR and more specifically the module relating to processing GeoJSON and WKT formats using the OpenLayers library. Here's my feedback:

1. Clarity & Readability 🌟

The code is clear and well-structured. Functions are named descriptively, which makes the module easy to understand even for someone unfamiliar with the domain specifics.

2. Code Duplication 🔍

There's a pattern in both geojson and wkt functions where we push a value into field_stats. While this isn't strictly duplicate code due to the different data structures being processed, it may be worth keeping an eye on this if further operations get added in the future. Consider further refactoring if this becomes more complex.

3. Documentation 📝

Consider adding more comments to explain the purpose of each function, the expected format of its parameters, and any other quirks or nuances that might help a developer unfamiliar with this module.

4. Use of Utility Function 👍

The introduction of the initializeFieldStats utility function is a commendable move. This ensures that any initialization related logic is contained within its own function, thus keeping the main functions clean and adhering to the Single Responsibility Principle.:+1:

In Conclusion

The module is well-structured and makes good use of utility functions to minimize redundancy. Great work overall! 🚀

@eo-uk
Copy link
Contributor

eo-uk commented Aug 18, 2023

vector.mjs:101 throws below exception with certain url params:

Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'getFeatures')

@sonarcloud
Copy link

sonarcloud bot commented Aug 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@dbauszus-glx dbauszus-glx marked this pull request as draft August 18, 2023 15:40
@dbauszus-glx dbauszus-glx marked this pull request as ready for review August 18, 2023 16:07
@RobAndrewHurst RobAndrewHurst merged commit d3c103f into GEOLYTIX:main Aug 21, 2023
6 checks passed
@RobAndrewHurst RobAndrewHurst added the Code Issues related to the code structure and performance. label Sep 4, 2023
@dbauszus-glx dbauszus-glx deleted the multivariate-grid branch January 29, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Issues related to the code structure and performance.
Projects
None yet
3 participants