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

migrate rail component styles to use utils #621

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

UdayHyma
Copy link

@UdayHyma UdayHyma commented Jan 7, 2025

User description

  • Create rail.module.css for component-specific styles
  • Add data-appearance and data-emphasis attributes

PR Type

Enhancement


Description

  • Migrated inline styles to CSS module for Rail component.

  • Added clsx for conditional class name management.

  • Introduced data-appearance and data-emphasis attributes for styling.

  • Created rail.module.css for component-specific styles.


Changes walkthrough 📝

Relevant files
Enhancement
rail.tsx
Refactor `Rail` component to use CSS modules                         

packages/ui/src/components/rail.tsx

  • Replaced inline styles with CSS module classes.
  • Added clsx for managing class names dynamically.
  • Introduced data-appearance and data-emphasis attributes.
  • +5/-9     
    rail.module.css
    Add CSS module for `Rail` component styles                             

    packages/ui/src/components/rail.module.css

  • Added .stack class for padding and border-radius styles.
  • Created a new CSS module for Rail component-specific styles.
  • +4/-0     

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

    - Create rail.module.css for component-specific styles
    - Add data-appearance and data-emphasis attributes
    Copy link

    changeset-bot bot commented Jan 7, 2025

    ⚠️ No Changeset found

    Latest commit: e82d28f

    Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

    This PR includes no changesets

    When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

    Click here to learn what changesets are, and how to add one.

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

    Copy link
    Contributor

    github-actions bot commented Jan 7, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Styling Consistency

    Ensure that the new clsx usage and rail.module.css styles maintain the same visual appearance and functionality as the previous inline styles.

    className={clsx(styles.rail, 'ts-canvas')}
    data-appearance='neutral'
    data-emphasis='minimal'
    CSS Utility Alignment

    Verify that the new CSS utility classes in rail.module.css align with the existing design system and do not introduce inconsistencies.

    .stack {
        padding: var(--component-spacing-md);
        border-radius: var(--component-radii-md);
      }

    Copy link
    Contributor

    github-actions bot commented Jan 7, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Define the missing .rail class to ensure proper styling for the component

    Add a .rail class definition in rail.module.css to ensure the styles.rail reference
    in the JSX file is valid and styles are applied correctly.

    packages/ui/src/components/rail.module.css [1-4]

    -.stack {
    -    padding: var(--component-spacing-md);
    -    border-radius: var(--component-radii-md);
    +.rail {
    +    background: var(--color-neutral-canvas-minimal-bg);
    +    border-right: 1px solid var(--color-neutral-stroke-default);
    +    color: var(--color-neutral-canvas-minimal-fg-default);
    +    height: 100%;
    +    padding: var(--component-spacing-xl) var(--component-spacing-md) var(--component-spacing-md);
       }
    Suggestion importance[1-10]: 9

    Why: Adding the .rail class is crucial since it is referenced in the JSX file. Without this definition, the component will lack the intended styles, leading to potential UI issues. This suggestion directly addresses a missing critical style.

    9
    Verify that the referenced CSS class exists to prevent runtime issues

    Ensure that the styles.rail class in the clsx function is correctly defined in the
    rail.module.css file to avoid runtime errors or missing styles.

    packages/ui/src/components/rail.tsx [105]

    -className={clsx(styles.rail, 'ts-canvas')}
    +className={clsx(styles?.rail, 'ts-canvas')}
    Suggestion importance[1-10]: 7

    Why: The suggestion to use optional chaining (styles?.rail) is valid as it ensures the code does not throw an error if styles.rail is undefined. This is particularly useful in dynamic or modular CSS setups, improving robustness.

    7

    Added the correct css properties to the modules file
    @@ -0,0 +1,7 @@
    .stack {
    background: var(--color-neutral-canvas-minimal-bg);
    border-right: 1px solid var(--color-neutral-stroke-default);
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    this should be var(--component-border-width-md) solid var(--stroke-default)

    i would change the stroke-default to subtle

    @@ -0,0 +1,7 @@
    .stack {
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    call the class rail to be more explicit

    padding:
    'var(--component-spacing-xl) var(--component-spacing-md) var(--component-spacing-md)'
    }}
    className={clsx(styles.stack, 'ts-canvas')}
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    update the class here

    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.

    2 participants