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

color harmonie node #631

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

color harmonie node #631

wants to merge 1 commit into from

Conversation

mck
Copy link
Collaborator

@mck mck commented Jan 10, 2025

User description

  • Supports multiple harmony types:
    • Analogous (±30°)
    • Complementary (180°)
    • Split-complementary (±150°)
    • Triadic (±120°)
    • Tetradic (90°, 180°, 270°)
  • Configurable number of output colors with pattern repetition
  • Preserves original color's saturation, lightness, and alpha values
  • Works with any input color space (converts to HSL internally)

PR Type

Enhancement, Tests


Description

  • Introduced a new Color Harmonies node for generating harmonious color combinations.

  • Supports multiple harmony types: analogous, complementary, split-complementary, triadic, and tetradic.

  • Added comprehensive tests for all harmony types and edge cases.

  • Ensures preservation of saturation, lightness, and alpha values in generated colors.


Changes walkthrough 📝

Relevant files
Enhancement
harmonies.ts
Introduced `Color Harmonies` node with harmony generation logic

packages/graph-engine/src/nodes/color/harmonies.ts

  • Added a new Color Harmonies node for generating harmonious color
    combinations.
  • Supports five harmony types with configurable number of colors.
  • Ensures saturation, lightness, and alpha values are preserved.
  • Implements logic for hue shifts and pattern repetition.
  • +134/-0 
    Configuration changes
    index.ts
    Registered `Color Harmonies` node in index                             

    packages/graph-engine/src/nodes/color/index.ts

  • Registered the new Color Harmonies node in the color nodes index.
  • Ensured proper ordering of node exports.
  • +10/-8   
    Tests
    harmonies.test.ts
    Added tests for `Color Harmonies` node functionality         

    packages/graph-engine/tests/suites/nodes/color/harmonies.test.ts

  • Added tests for all harmony types: analogous, complementary,
    split-complementary, triadic, and tetradic.
  • Verified correct hue shifts and pattern repetition.
  • Tested preservation of saturation, lightness, and alpha values.
  • +149/-0 
    Documentation
    strong-rules-push.md
    Added changeset for `Color Harmonies` node                             

    .changeset/strong-rules-push.md

  • Documented the addition of the Color Harmonies node.
  • Highlighted supported harmony types and key features.
  • +12/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @mck mck self-assigned this Jan 10, 2025
    Copy link

    changeset-bot bot commented Jan 10, 2025

    🦋 Changeset detected

    Latest commit: b707dfe

    The changes in this PR will be included in the next version bump.

    This PR includes changesets to release 1 package
    Name Type
    @tokens-studio/graph-engine Minor

    Not sure what this means? Click here to learn what changesets are.

    Click here if you're a maintainer who wants to add another changeset to this PR

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Logic Issue

    The logic for handling hue shifts in the execute method should be reviewed to ensure that all harmony types produce the correct and expected results. Specifically, verify that the hue adjustments and modulo operations are accurate for edge cases.

    let hueShifts: number[] = [];
    switch (harmonyType) {
    	case 'analogous':
    		hueShifts = [-30, 30];
    		break;
    	case 'complementary':
    		hueShifts = [180];
    		break;
    	case 'splitComplementary':
    		hueShifts = [150, -150];
    		break;
    	case 'triadic':
    		hueShifts = [120, -120];
    		break;
    	case 'tetradic':
    		hueShifts = [90, 180, -90];
    		break;
    }
    
    // Start with the base color
    harmonies.push(color);
    
    // Generate harmony colors
    for (const shift of hueShifts) {
    	let newHue = (baseHue + shift) % 360;
    	if (newHue < 0) newHue += 360;
    
    	const newColor = new Color(
    		'hsl',
    		[newHue, saturation, lightness],
    		color.alpha
    	);
    	harmonies.push(toColorObject(newColor));
    }
    
    // Ensure we return the requested number of colors
    if (harmonies.length > numberOfColors) {
    	harmonies = harmonies.slice(0, numberOfColors);
    } else if (harmonies.length < numberOfColors) {
    	const originalLength = harmonies.length;
    	for (let i = originalLength; i < numberOfColors; i++) {
    		harmonies.push(harmonies[i % originalLength]);
    	}
    Repetition Logic

    The logic for repeating colors to meet the numberOfColors requirement should be reviewed to ensure it handles edge cases correctly, such as when the requested number of colors is less than or equal to the base harmonies.

    if (harmonies.length > numberOfColors) {
    	harmonies = harmonies.slice(0, numberOfColors);
    } else if (harmonies.length < numberOfColors) {
    	const originalLength = harmonies.length;
    	for (let i = originalLength; i < numberOfColors; i++) {
    		harmonies.push(harmonies[i % originalLength]);
    	}

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation for numberOfColors to ensure it is a positive integer

    Validate that numberOfColors is a positive integer before using it to slice or
    repeat the harmonies array to avoid unexpected behavior or infinite loops.

    packages/graph-engine/src/nodes/color/harmonies.ts [123-129]

    -if (harmonies.length > numberOfColors) {
    -    harmonies = harmonies.slice(0, numberOfColors);
    -} else if (harmonies.length < numberOfColors) {
    -    const originalLength = harmonies.length;
    -    for (let i = originalLength; i < numberOfColors; i++) {
    -        harmonies.push(harmonies[i % originalLength]);
    +if (numberOfColors > 0 && Number.isInteger(numberOfColors)) {
    +    if (harmonies.length > numberOfColors) {
    +        harmonies = harmonies.slice(0, numberOfColors);
    +    } else if (harmonies.length < numberOfColors) {
    +        const originalLength = harmonies.length;
    +        for (let i = originalLength; i < numberOfColors; i++) {
    +            harmonies.push(harmonies[i % originalLength]);
    +        }
         }
    +} else {
    +    throw new Error('numberOfColors must be a positive integer');
     }
    Suggestion importance[1-10]: 10

    Why: Validating numberOfColors as a positive integer is essential to prevent logical errors or infinite loops. This suggestion significantly improves the reliability and safety of the code by handling invalid input gracefully.

    10
    Add a fallback for the alpha value to prevent runtime errors if it is undefined or null

    Ensure that the color.alpha property is checked for undefined or null values before
    using it in the new Color constructor to avoid potential runtime errors.

    packages/graph-engine/src/nodes/color/harmonies.ts [114-118]

     const newColor = new Color(
         'hsl',
         [newHue, saturation, lightness],
    -    color.alpha
    +    color.alpha ?? 1
     );
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by providing a default value for the alpha channel when it is undefined or null. It ensures robustness and prevents unexpected crashes, which is critical for the functionality of the code.

    9
    General
    Add a test case to ensure invalid numberOfColors values are handled correctly

    Add a test case to verify the behavior when numberOfColors is set to zero or a
    negative value to ensure the function handles invalid input gracefully.

    packages/graph-engine/tests/suites/nodes/color/harmonies.test.ts [108-126]

    -test('handles custom number of colors by repeating', async () => {
    +test('handles invalid numberOfColors gracefully', async () => {
         const graph = new Graph();
         const node = new Node({ graph });
     
         node.inputs.harmonyType.setValue('triadic');
    -    node.inputs.numberOfColors.setValue(5);
    +    node.inputs.numberOfColors.setValue(-1);
     
    -    await node.execute();
    -    const colors = node.outputs.colors.value as ColorType[];
    -
    -    expect(colors).toHaveLength(5);
    -    // Should repeat the pattern: base, +120, -120, base, +120
    -    const expectedHues = [0, 120, 240, 0, 120];
    -    colors.forEach((color, index) => {
    -        const hslColor = toColor(color).to('hsl');
    -        expect(Math.round(hslColor.coords[0])).toBe(expectedHues[index]);
    -    });
    +    await expect(node.execute()).rejects.toThrow('numberOfColors must be a positive integer');
     });
    Suggestion importance[1-10]: 8

    Why: Adding a test case for invalid numberOfColors values enhances test coverage and ensures the function behaves as expected under edge cases. This is important for maintaining code quality and reliability.

    8

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

    Successfully merging this pull request may close these issues.

    1 participant