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

fix: Skeleton scrollbar issue #750

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

chetan102
Copy link

@chetan102 chetan102 commented Feb 12, 2025

User description

fixed #744 scrollbar issue when its in loading state


PR Type

Bug fix


Description

  • Fixed unnecessary scrollbar during skeleton loading by adjusting container height.

  • Improved UI consistency for skeleton loaders with h-full class.

  • Addressed potential overflow issues in the switch component.


Changes walkthrough 📝

Relevant files
Bug fix
page.tsx
Adjusted container height to prevent scrollbar                     

apps/platform/src/app/(main)/page.tsx

  • Added h-full class to the main container for consistent height.
  • Ensured layout stability during skeleton loading.
  • +1/-1     
    index.tsx
    Updated loader container height for better UX                       

    apps/platform/src/components/dashboard/project/projectScreenLoader/index.tsx

  • Replaced min-h-screen with h-full for the loader container.
  • Improved layout behavior during skeleton loading.
  • +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    744 - Partially compliant

    Compliant requirements:

    • Remove unnecessary scrollbar during skeleton loading state
    • Fix scrollbar causing unintended UI shift

    Non-compliant requirements:

    • Fix switch component slider overflow issue
    • Use overflow:auto for skeleton loaders

    Requires further human verification:

    • Visual verification needed to confirm scrollbar behavior is fixed
    • Switch component fixes need to be validated as they are not visible in the code changes
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Missing Overflow Control

    The ticket specifically requests using overflow:auto but this is not implemented in the changes. Consider adding overflow control to prevent unwanted scrollbars.

    <div className="bg-background dark h-full p-8">

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add missing React key prop

    Add a key prop to the mapped elements to help React efficiently update the list
    and prevent potential rendering issues.

    apps/platform/src/components/dashboard/project/projectScreenLoader/index.tsx [7-9]

     {[...Array(6)].map((_, i) => (
       <div
    +    key={i}
         className="flex h-[7rem] items-center space-x-4 rounded-xl bg-white/5 p-4"
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: Missing key props in React list rendering can cause performance issues and bugs during re-renders. This is a critical React best practice that helps maintain component state and improve rendering efficiency.

    Medium

    @chetan102 chetan102 changed the title fix : Skeleton scrollbar issue fix: Skeleton scrollbar issue Feb 12, 2025
    @rajdip-b
    Copy link
    Member

    The issue is related to FOSSHack, are you sure you want to go ahead with the PR? Your contribution wont be accounted for in the hackathon

    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.

    PLATFORM: Unnecessary Scrollbar During Skeleton Loading & UI Issue with Switch
    2 participants