Fix issue: #963 UI Overflow on Mobile for Home page #1030
Conversation
Summary by CodeRabbit
WalkthroughThis pull request applies significant layout and component restructuring in the Home page. The outer container now features updated styling, including new margin, padding, and dark mode adjustments. The internal composition is reorganized with new wrappers and conditional rendering enhancements for event details, contributor counts, and comments. Specific sections like "Upcoming Events", "New Chapters", and "New Projects" have been restructured and merged accordingly, while the display of logos has been repositioned. No changes were made to any exported or public declarations. Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (7)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
frontend/src/pages/Home.tsx (1)
10-19:⚠️ Potential issueRemove unused import
EventType.The
EventTypeimport on line 18 is declared but never used in this file, as flagged by ESLint in the pipeline failure.- import { EventType } from 'types/event'🧰 Tools
🪛 GitHub Actions: Run CI/CD
[error] 10-10: 'EventType' is defined but never used @typescript-eslint/no-unused-vars
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/pages/Home.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run CI/CD
frontend/src/pages/Home.tsx
[error] 10-10: 'EventType' is defined but never used @typescript-eslint/no-unused-vars
🔇 Additional comments (5)
frontend/src/pages/Home.tsx (5)
111-113: Good container restructuring to fix mobile overflow.The changes to the container structure address the mobile overflow issue effectively. Moving from a fixed-width outer container to a padded container with a max-width inner wrapper is a good responsive design approach.
131-183: Good restructuring of chapter and project cards.The implementation of separate cards for new chapters and projects with appropriate formatting and consistent styling improves the layout organization and readability on mobile devices.
184-184: Appropriate reduction in displayed contributors.Reducing the
maxInitialDisplayfrom 9 to 6 forTopContributorsis a good adjustment that will help prevent overcrowding on mobile screens.
192-221: Good enhancement with conditional comment count display.The added conditional rendering for comment counts in the ItemCardList components is a nice improvement that prevents displaying empty or zero counts.
223-249: Good responsive grid layout for statistics.Using a responsive grid that switches from 1 column on mobile to 4 columns on medium screens ensures the statistics display properly on all device sizes.
KaranNegi20Feb
force-pushed
the
fixing-home-overflow
branch
2 times, most recently
from
88066b1 to
2a34af3
Compare
KaranNegi20Feb
force-pushed
the
fixing-home-overflow
branch
from
78a0449 to
886e785
Compare
|
|
Hey @arkid15r all the merge conflicts are resolved, you can now proceed with your review. Thank you. |
KaranNegi20Feb
force-pushed
the
fixing-home-overflow
branch
from
4f2d772 to
9d5f492
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/pages/Home.tsx (1)
113-113: Consider more responsive padding for the main container.The padding of
p-8(32px) might be excessive on very small mobile screens. Using responsive padding would provide better spacing across different device sizes.- <div className="mt-16 min-h-screen p-8 text-gray-600 dark:bg-[#212529] dark:text-gray-300"> + <div className="mt-16 min-h-screen p-4 md:p-8 text-gray-600 dark:bg-[#212529] dark:text-gray-300">
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/e2e/pages/Contribute.spec.ts(1 hunks)frontend/src/pages/Home.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/e2e/pages/Contribute.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (5)
frontend/src/pages/Home.tsx (5)
114-114: LGTM! Great solution for preventing overflow.Adding a centered, max-width container is an excellent approach to address the mobile overflow issue. This ensures content is properly constrained across all device sizes.
160-212: Good implementation of responsive grid.The two-column layout with
md:grid-cols-2properly collapses to a single column on mobile screens, preventing horizontal overflow. The consistent use of this pattern throughout the page is excellent for maintaining responsive behavior.
213-213: Reduced the initial contributors display count.The
maxInitialDisplayprop for TopContributors has been reduced from 9 to 6, which should help with mobile display by showing fewer items initially.
251-260: Good responsive implementation for counter stats.The grid layout with
md:grid-cols-4ensures the counter stats are stacked vertically on mobile screens and displayed in a row on larger screens, which is appropriate for mobile viewing.
277-279: Moving logos implementation preserved as requested.As noted in the past review comments, the MovingLogos component has been correctly preserved and properly integrated into the new layout structure.
welcome @aramattamara 😄. |
There was a problem hiding this comment.
It seems that there are still some changes to the home page that are not rellevant to this task.
-
We used to have 3 columns for
Upcoming events, now it shows two:
-
Recent issues and releases are still showing up in rows instead of having two columns side by side:
|
Fixed for 3 columns instead of 2 Screen.Recording.2025-03-09.at.10.14.48.AM.movnewone.mov |
KaranNegi20Feb
force-pushed
the
fixing-home-overflow
branch
from
6c6ab49 to
59a2ef2
Compare
|
Hey @aramattamara, @arkid15r and @kasya, I have tried to mitigate as may ui flaws in this pr as many as possible. Thankyou for your continuous guidance and support throughout this PR reviews. I also have mentioned the final outcomes above with the help of a video and screenshots, do go through those. Link below: |
There was a problem hiding this comment.
@KaranNegi20Feb I don't see those changes being applied🤔 Home page still looks the same for me.
Are you sure you pushed everything?
Yes the branch is updated, could you share your side of the view since i pushed all the changes |
|
Hey @kasya I checked the code push, everything seems fine here, the branch runs fine on my device as well, if you could show me what's that you saw that does not match the right version. Thank you. |
KaranNegi20Feb
force-pushed
the
fixing-home-overflow
branch
from
b01c252 to
ab7f5b9
Compare
|
|
This is how it looks on my local machine. Screen.Recording.2025-03-10.at.10.24.35.AM.mov |
Describe the bug:
The page width and the container width do not match properly, causing horizontal overflow on mobile devices.
Fix results:
Fix_mobile.mov