Skip to content

Arch/modularize artwork asset registry#6534

Open
vanshika2720 wants to merge 1 commit intosugarlabs:masterfrom
vanshika2720:arch/modularize-artwork-asset-registry
Open

Arch/modularize artwork asset registry#6534
vanshika2720 wants to merge 1 commit intosugarlabs:masterfrom
vanshika2720:arch/modularize-artwork-asset-registry

Conversation

@vanshika2720
Copy link
Copy Markdown
Contributor

@vanshika2720 vanshika2720 commented Apr 10, 2026

PR Description: Modularize artwork.js into a Managed AssetRegistry

Overview

This PR refactors the monolithic js/artwork.js file to eliminate global namespace pollution and provide a modern, centralized API for visual asset management. By moving from ad-hoc regex replacements to a structured AssetRegistry, we improve code maintainability and solve the long-standing "architectural bottleneck" associated with SVG handling in Music Blocks.

Key Changes

  • Implemented AssetRegistry: A central repository in js/artwork.js for all SVG assets and visual constants.
  • Implemented getSVG(iconName, options): A new, consistent API for retrieving and dynamically styling SVGs (supporting fillColor, strokeColor, and arbitrary placeholders).
  • Migrated Major Consumers: Successfully updated turtles.js, activity.js, EnsembleBlocks.js, turtle-painter.js, trash.js, themebox.js, and palette.js to use the new API.
  • Dependency Cleanup: Heavily reduced the global variable dependencies in multiple core files.
  • Expanded Testing: Added comprehensive unit tests for the new registry and API in js/__tests__/artwork.test.js.
  • Lint Fixes: Resolved existing ESLint warnings in touched files to ensure compatibility with strict pre-commit hooks.

How to Test

  1. Run npm test js/__tests__/artwork.test.js to verify the new API.
  2. Run npm test js/blocks/__tests__/EnsembleBlocks.test.js to ensure block rendering logic is preserved.
  3. Open Music Blocks in a browser and verify that turtle shells, palette icons, and the trash basket render correctly with their expected colors.

Goal

Reduce the memory footprint of the global namespace and modernize visual asset management for Music Blocks v4.

  • Performance

@github-actions
Copy link
Copy Markdown
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

palette.test.js
trash.test.js

@github-actions github-actions bot added performance Improves performance (load time, memory, rendering) size/L Large: 250-499 lines changed area/javascript Changes to JS source files area/docs Changes to documentation labels Apr 10, 2026
@vanshika2720
Copy link
Copy Markdown
Contributor Author

still working/testing

@github-actions github-actions bot added the area/tests Changes to test files label Apr 10, 2026
@vanshika2720 vanshika2720 force-pushed the arch/modularize-artwork-asset-registry branch from e950dcc to 3c3585a Compare April 10, 2026 15:48
@github-actions github-actions bot removed the area/docs Changes to documentation label Apr 10, 2026
@github-actions
Copy link
Copy Markdown
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

palette.test.js
trash.test.js

@vanshika2720 vanshika2720 force-pushed the arch/modularize-artwork-asset-registry branch from 3c3585a to b52259b Compare April 10, 2026 17:01
@github-actions
Copy link
Copy Markdown
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@github-actions github-actions bot added size/XL Extra large: 500-999 lines changed and removed size/L Large: 250-499 lines changed labels Apr 11, 2026
@github-actions
Copy link
Copy Markdown
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

artwork_integration.test.js
themebox.test.js

@github-actions
Copy link
Copy Markdown
Contributor

✅ All Jest tests passed! This PR is ready to merge.

1 similar comment
@github-actions
Copy link
Copy Markdown
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@github-actions
Copy link
Copy Markdown
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

artwork_integration.test.js

@github-actions
Copy link
Copy Markdown
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@vanshika2720 vanshika2720 force-pushed the arch/modularize-artwork-asset-registry branch from 8926273 to efac971 Compare April 12, 2026 02:39
@github-actions
Copy link
Copy Markdown
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

toolbar.test.js

@vanshika2720 vanshika2720 force-pushed the arch/modularize-artwork-asset-registry branch from efac971 to f4edbdd Compare April 12, 2026 02:42
@github-actions
Copy link
Copy Markdown
Contributor

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

toolbar.test.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/javascript Changes to JS source files area/tests Changes to test files performance Improves performance (load time, memory, rendering) size/XL Extra large: 500-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant