-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: remove duplicate logic and consolidate utility functions #108
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: HendrikThePendric <353236+HendrikThePendric@users.noreply.github.com>
Co-authored-by: HendrikThePendric <353236+HendrikThePendric@users.noreply.github.com>
Co-authored-by: HendrikThePendric <353236+HendrikThePendric@users.noreply.github.com>
Co-authored-by: HendrikThePendric <353236+HendrikThePendric@users.noreply.github.com>
Co-authored-by: HendrikThePendric <353236+HendrikThePendric@users.noreply.github.com>
Co-authored-by: HendrikThePendric <353236+HendrikThePendric@users.noreply.github.com>
Co-authored-by: HendrikThePendric <353236+HendrikThePendric@users.noreply.github.com>
Co-authored-by: HendrikThePendric <353236+HendrikThePendric@users.noreply.github.com>
Co-authored-by: HendrikThePendric <353236+HendrikThePendric@users.noreply.github.com>
Co-authored-by: HendrikThePendric <353236+HendrikThePendric@users.noreply.github.com>
Co-authored-by: HendrikThePendric <353236+HendrikThePendric@users.noreply.github.com>
Co-authored-by: HendrikThePendric <353236+HendrikThePendric@users.noreply.github.com>
HendrikThePendric
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes are looking good so far. @copilot could you check one specific file: src/components/app-wrapper/metadata-helpers/visualization.ts. I think we could be doing the same thing a few times there. Also, please carefully check the files in the src/modules dir, these have all been ported and renamed over several PRs by 3 different people. The risk that we ended up with overlapping logic there seems quite significant.
|
🚀 Deployed on https://pr-108.event-visualizer.netlify.dhis2.org |
Co-authored-by: HendrikThePendric <353236+HendrikThePendric@users.noreply.github.com>
Reviewed both files as requested. Found and removed duplicate code in
The Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
I'm unable to reword that commit as it has already been pushed to the remote branch and I don't have access to force push. The commit would need to be amended manually or via an interactive rebase by someone with push access. All subsequent commits in this PR follow the conventional commit format (e.g., |
Implements DHIS2-20136
Description
Addresses duplicate code and unnecessary complexity introduced during the port from line-listing-app.
Consolidated Options Constants (
src/constants/options.ts)OPTIONS_SECTION_KEYS_LINE_LISTandOPTIONS_SECTION_KEYS_PIVOT_TABLEwere identical arrays, now sharing a single sourceCentralized Header Map Utilities (
src/modules/visualization.ts)reversedHeadersMap- pre-computed reverse map for O(1) lookupsgetDimensionIdFromHeaderName()utility using the pre-computed mapObject.keys(headersMap).find()pattern from 4 locationsRemoved Duplicate Code in Metadata Helpers (
src/components/app-wrapper/metadata-helpers/visualization.ts)DIMENSION_METADATA_PROP_MAP- now importsdimensionMetadataPropMapfrom@modules/visualizationgetDefaultOrgUnitMetadataandgetDefaultOrgUnitLabel- now imports from@modules/metadataBefore:
After:
Quality checklist
ToDos
Known issues
isSubsetOf is not a function) unrelated to these changes - Node.js version compatibility issue with ES2023 Set methodScreenshots
N/A - refactoring only, no UI changes
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.