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

Revert Icon Legend module #1205

Merged
merged 4 commits into from
Apr 11, 2024
Merged

Conversation

RobAndrewHurst
Copy link
Contributor

@RobAndrewHurst RobAndrewHurst commented Apr 11, 2024

Revert & Refactor legendIcon Function to Improve Code Readability and Maintainability

This pull request contains changes to refactor the legendIcon function in the /ui/elements/legendIcon module. The main goal of the PR was to revert the module to a previous working version and to refactor the module to improve code readability, maintainability, and reduce cognitive complexity.

Changes ✍️

  1. Refactored the legendIcon function to reduce cognitive complexity:

    • Extracted the icon array handling logic into a separate function called createIconFromArray.
    • Moved the icon creation logic for non-array icons to a separate function called createIconFromInlineStyle.
    • Extracted the line symbol creation logic into a separate function called createLineSymbol.
    • Extracted the polygon symbol creation logic into a separate function called createPolygonSymbol.
    • Simplified the conditional statements in the main legendIcon function by using early returns.
  2. Added JSDoc comments for better documentation 📝 :

    • Provided a description of the legendIcon function and its parameters.
    • Added comments to explain the purpose and functionality of each separate function.
  3. Improved code readability and maintainability 👀 :

    • Separated the complex logic into smaller, more focused functions.
    • Used meaningful function names to describe the purpose of each function.
    • Added comments to clarify the different branches and conditions in the code.

Benefits 👍

  • Reduced cognitive complexity 🧠 :

    • The refactoring has brought down the cognitive complexity of the legendIcon function from 18 to within the allowed limit of 15.
    • The extraction of separate functions for icon array handling, icon creation, line symbol creation, and polygon symbol creation has made the code easier to understand and maintain.
  • Improved code readability and maintainability 🔧 :

    • The separation of concerns into smaller functions has made the code more modular and easier to read.
    • The added comments provide clear explanations of the purpose and functionality of each function.
    • The meaningful function names make it easier to understand the intent of each function at a glance.
  • Enhanced documentation:

    • The JSDoc comments provide a clear description of the legendIcon function and its parameters.
    • The comments added to each separate function explain their purpose and functionality.

Testing 🧪

  • The refactored code has been roughly tested to ensure that it produces the same results as the original code.
  • Test cases have been run to verify that icons, line symbols, and polygon symbols are generated correctly based on the provided style parameters.

@RobAndrewHurst RobAndrewHurst added the Bug A genuine bug. There must be some form of error exception to work with. label Apr 11, 2024
@RobAndrewHurst RobAndrewHurst marked this pull request as draft April 11, 2024 10:06
Copy link

sonarcloud bot commented Apr 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@AlexanderGeere
Copy link
Contributor

Using icon.anchor on the legend icons, moves them out of view.

Copy link
Contributor

@AlexanderGeere AlexanderGeere left a comment

Choose a reason for hiding this comment

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

Checked out a few instances, all looks well to me.

Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

Tested a few myself too and happy its working as expected!
Tested in reports too and using anchors and legendScale

@RobAndrewHurst RobAndrewHurst merged commit 76f1934 into GEOLYTIX:main Apr 11, 2024
5 checks passed
@RobAndrewHurst RobAndrewHurst deleted the icon_legend branch April 11, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A genuine bug. There must be some form of error exception to work with.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants