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.

Sign up for GitHub

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

Jump to bottom

fixed layout of handle names and live types #640

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fixed layout of handle names and live types #640

wants to merge 3 commits into from

Conversation

AlexBxl
Copy link
Contributor

@AlexBxl AlexBxl commented Jan 16, 2025 •
edited by github-actions bot
Loading

User description

Description

  • restyled handle names and live types
  • nameless ports no longer collapse into each other
  • special handling in CSS for live types in dark mode (should probably be done in DS)

Fixes #638

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Video

f041bf83-8e8c-49c6-8ab8-8c703a1cebed

PR Type

Bug fix, Enhancement

Description

  • Adjusted font size for handle names for better readability.

  • Improved CSS styling for handle holders and live types.

    • Added conditional margin adjustments for nameless ports.
    • Enhanced dark mode styling for live type labels.

  • Updated inline type label styles for better visibility.

  • Increased handle core size for better usability.

Changes walkthrough 📝

Relevant files
Enhancement
nodeV2.tsx
Adjusted font size for handle names                                           

packages/graph-editor/src/components/flow/wrapper/nodeV2.tsx

  • Updated font size for handle names to var(--font-code-lg).
 +1/-1     
handles.module.css
Enhanced handle holder and core styles                                     

packages/graph-editor/src/components/flow/handles.module.css

  • Added margin adjustments for nameless ports.
  • Increased handle core size to var(--size-75).
  • Improved CSS for collapsed handle holders.
    		• +7/-1     
    nodeV2.module.css
    Improved inline type label styles                                               

    packages/graph-editor/src/components/flow/wrapper/nodeV2.module.css

  • Updated background and padding for inline type labels.
  • Added dark mode-specific styles for live type labels.
  • Adjusted font size and weight for better visibility.
    		• +7/-4     

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

    @changeset-bot changeset-bot
    Copy link

    changeset-bot bot commented Jan 16, 2025 •
    edited
    Loading

    🦋 Changeset detected

    Latest commit: e1d1c9e

    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-editor Patch

    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

    @github-actions GitHub Actions
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    638 - Fully compliant

    Fully compliant requirements:

    • Fix live value CSS styles in nodes.
    • Ensure nameless ports do not collapse into each other.
    • Add special handling for live types in dark mode.

    Not compliant requirements:
    []

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

    Possible Issue

    The conditional margin adjustment for nameless ports (:not(:has(.handleText, span))) should be tested to ensure it works as expected across all browsers and scenarios.

    &:not(:has(.handleText, span)) {
  margin-bottom: var(--component-spacing-lg);
    Dark Mode Styling

    The new dark mode styling for .inlineTypeLabel should be validated for proper contrast and readability.

    :global(.ts-theme-dark) .inlineTypeLabel {
    background: rgba(from var(--color-neutral-surface-default-idle-bg) r g b / 0.8);
    font-weight: 800;

    @github-actions GitHub Actions
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Adjust the font size to prevent potential layout inconsistencies

    Ensure that the new font size var(--font-code-lg) is appropriate for all contexts
    where input.name is displayed, as it might cause layout issues or inconsistencies
    with other UI elements.

    packages/graph-editor/src/components/flow/wrapper/nodeV2.tsx [398]

    -font: 'var(--font-code-lg)',
+font: 'var(--font-code-md)',
    Suggestion importance[1-10]: 6

    Why: The suggestion to adjust the font size from var(--font-code-lg) to var(--font-code-md) is reasonable as it could help prevent layout inconsistencies. However, it is not critical and depends on the specific design requirements and context of input.name.

    		6
    Adjust margin rules to avoid unintended spacing issues

    Verify that the new margin-bottom rules for .handleHolder do not create unintended
    spacing issues, especially when .handleText or span elements are not present.

    packages/graph-editor/src/components/flow/handles.module.css [8-9]

     &:not(:has(.handleText, span)) {
-  margin-bottom: var(--component-spacing-lg);
+  margin-bottom: var(--component-spacing-md);
 }
    Suggestion importance[1-10]: 5

    Why: The suggestion to modify the margin-bottom value for .handleHolder is valid as it could help avoid unintended spacing issues. However, it is not a critical change and requires verification in the context of the UI layout.

    		5
    Adjust dark theme styles to ensure consistency and avoid conflicts

    Verify that the new dark theme styles for .inlineTypeLabel do not conflict with
    other theme-specific styles or cause visual inconsistencies.

    packages/graph-editor/src/components/flow/wrapper/nodeV2.module.css [24-27]

     :global(.ts-theme-dark) .inlineTypeLabel {
-    background: rgba(from var(--color-neutral-surface-default-idle-bg) r g b / 0.8);
-    font-weight: 800;
+    background: rgba(from var(--color-neutral-surface-default-idle-bg) r g b / 0.7);
+    font-weight: 700;
 }
    Suggestion importance[1-10]: 5

    Why: The suggestion to slightly adjust the dark theme styles for .inlineTypeLabel is valid and could improve consistency. However, the impact is moderate, and the current styles might already be acceptable depending on the overall theme design.

    		5
    Improve background transparency for better readability

    Test the new rgba background color with transparency to ensure it maintains
    sufficient contrast and readability across all themes and backgrounds.

    packages/graph-editor/src/components/flow/wrapper/nodeV2.module.css [8]

    -background: rgba(from var(--color-neutral-surface-default-idle-bg) r g b / 0.65);
+background: rgba(from var(--color-neutral-surface-default-idle-bg) r g b / 0.75);
    Suggestion importance[1-10]: 4

    Why: The suggestion to increase the transparency of the background color is minor and could improve readability slightly. However, the impact is limited, and the current value might already be sufficient depending on the design.

    		4

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers
    No reviews
    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    fix live value CSS styles in nodes
    1 participant
    @AlexBxl