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

💄 Show Indexes instead of Unique #964

Merged
merged 6 commits into from
Mar 28, 2025
Merged

Conversation

ya2s
Copy link
Contributor

@ya2s ya2s commented Mar 22, 2025

Issue

The Unique section is no longer needed, so replace it with Indexes

CleanShot 2025-03-22 at 23 27 44@2x

  • resolve:

Why is this change needed?

What would you like reviewers to focus on?

Testing Verification

What was done

🤖 Generated by PR Agent at 1a4795a

  • Replaced Unique section with Indexes in the UI.
  • Added new IndexesItem component for displaying index details.
  • Removed redundant Unique component and associated styles.
  • Updated icons and styles to support the new Indexes feature.

Detailed Changes

Relevant files
Enhancement
6 files
index.ts
Export `IndexesItem` component for reuse                                 
+1/-0     
index.ts
Add `FileText` icon export                                                             
+1/-0     
IndexesItem.module.css
Add styles for `IndexesItem` component                                     
+64/-0   
IndexesItem.tsx
Implement `IndexesItem` component for displaying index details
+46/-0   
Indices.tsx
Update `Indices` component to use `IndexesItem`                   
+9/-20   
FileText.tsx
Add `FileText` icon component                                                       
+8/-0     
Bug fix
5 files
index.ts
Remove export for `Unique` component                                         
+0/-1     
Indices.module.css
Remove obsolete styles for `Indices` component                     
+0/-16   
Unique.module.css
Remove obsolete styles for `Unique` component                       
+0/-16   
TableDetail.tsx
Remove `Unique` component reference from `TableDetail`     
+0/-2     
Unique.tsx
Remove `Unique` component implementation                                 
+0/-44   
Documentation
1 files
slimy-fishes-applaud.md
Add changeset for replacing `Unique` with `Indexes`           
+5/-0     

Additional Notes


Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @ya2s ya2s requested a review from a team as a code owner March 22, 2025 14:28
    @ya2s ya2s requested review from hoshinotsuyoshi, FunamaYukina, junkisai, MH4GF and NoritakaIkeda and removed request for a team March 22, 2025 14:28
    Copy link

    changeset-bot bot commented Mar 22, 2025

    🦋 Changeset detected

    Latest commit: 347c7b5

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

    This PR includes changesets to release 3 packages
    Name Type
    @liam-hq/ui Patch
    @liam-hq/erd-core Patch
    @liam-hq/cli 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

    Copy link

    vercel bot commented Mar 22, 2025

    The latest updates on your projects. Learn more about Vercel for Git ↗︎

    Name Status Preview Comments Updated (UTC)
    liam-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 28, 2025 1:50pm
    liam-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 28, 2025 1:50pm
    liam-erd-sample ✅ Ready (Inspect) Visit Preview Mar 28, 2025 1:50pm
    3 Skipped Deployments
    Name Status Preview Comments Updated (UTC)
    test-liam-app ⬜️ Ignored (Inspect) Mar 28, 2025 1:50pm
    test-liam-docs ⬜️ Ignored (Inspect) Mar 28, 2025 1:50pm
    test-liam-erd-sample ⬜️ Ignored (Inspect) Mar 28, 2025 1:50pm

    Copy link

    vercel bot commented Mar 22, 2025

    @ya2s is attempting to deploy a commit to the ROUTE06 Core Team on Vercel.

    A member of the Team first needs to authorize it.

    Copy link
    Contributor

    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

    Hardcoded Height

    The contentMaxHeight calculation uses a fixed value of 400 per index item, which seems excessive and may cause layout issues with many indexes.

    const contentMaxHeight = Object.keys(indices).length * 400
    
    Unused Import

    The Column type is imported from @liam-hq/db-structure but never used in the component.

    import type { Column } from '@liam-hq/db-structure'
    

    Copy link
    Contributor

    qodo-merge-pro-for-open-source bot commented Mar 22, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Remove unused export file

    This file contains a duplicate export line and the component it's exporting has
    been removed from the codebase (as shown by the removal of Unique.tsx). This
    file should be removed entirely since the Unique component is no longer used.

    frontend/packages/erd-core/src/features/erd/components/ERDContent/components/TableNode/TableDetail/Unique/index.ts [1]

    -export * from './Unique'
    +// This file should be removed entirely as the Unique component is no longer used

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: The file contains an export for a component that has been completely removed from the codebase (Unique.tsx). The PR shows the Unique component is being removed, but this export file was left behind, which could cause confusion and potential import errors.

    Medium
    General
    Improve height calculation

    The fixed height multiplier of 400 pixels per index item seems excessive and
    could cause layout issues with many indices. Consider using a more reasonable
    height calculation or implementing dynamic height calculation based on content.

    frontend/packages/erd-core/src/features/erd/components/ERDContent/components/TableNode/TableDetail/Indices/Indices.tsx [12]

    -const contentMaxHeight = Object.keys(indices).length * 400
    +// Use a more reasonable height estimate per index item
    +const contentMaxHeight = Object.keys(indices).length * 100
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The current implementation uses a fixed multiplier of 400 pixels per index item, which is likely excessive and could cause layout issues when there are many indices. A more reasonable height calculation would improve the UI's responsiveness and prevent potential overflow issues.

    Medium
    • Update

    Copy link
    Member

    @MH4GF MH4GF left a comment

    Choose a reason for hiding this comment

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

    It looks like there's no problem with the implementation.
    It's just that the lint is falling, so please check it!

    @ya2s ya2s force-pushed the feat/yass/add-indexes branch from d82dbea to 13a1d67 Compare March 27, 2025 14:58
    Copy link
    Member

    @MH4GF MH4GF left a comment

    Choose a reason for hiding this comment

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

    LGTM 👍🏻 Thanks!!

    @MH4GF
    Copy link
    Member

    MH4GF commented Mar 28, 2025

    @ya2s Please resolve conflict?

    @ya2s ya2s added this pull request to the merge queue Mar 28, 2025
    Merged via the queue into liam-hq:main with commit 491ca62 Mar 28, 2025
    13 checks passed
    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