feat(website): add MLH banner to landing page #243
Conversation
|
Warning Rate limit exceeded@JeremyFriesenGitHub has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant NavbarContainer
participant NavbarPresenter
participant Banner
NavbarContainer->>NavbarPresenter: Pass banner prop
NavbarPresenter->>Banner: Render Banner component
Banner-->>NavbarPresenter: Display banner links
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
![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)
libs/website/shared/ui/navigation/navbar/ui/navbar.presenter.stories.tsx (1)
Line range hint
59-119: Update placeholder social media URLs.The social media links are using generic URLs (e.g., 'https://github.com') instead of the actual cuHacking URLs from navbar.constants.ts.
Apply this diff to fix the URLs:
- link: 'https://github.com', + link: 'https://github.com/cuhacking/2025', - link: 'https://instagram.com', + link: 'https://www.instagram.com/cuhacking/', - link: 'https://linkedin.com', + link: 'https://ca.linkedin.com/company/cuhacking', - link: 'https://linktr.ee', + link: 'https://linktr.ee/cuhacking_', - link: 'mailto:[email protected]', + link: 'mailto:[email protected]', - link: 'https://discord.com', + link: 'https://discord.com/invite/h2cQqF9aZf', - link: 'https://docs.com', + link: 'https://docs.cuhacking.ca/', - link: 'https://figma.com', + link: 'https://www.figma.com/design/wc1JOWR48tBNkjcjwY3AzB/%E2%8C%A8%EF%B8%8F-cuHacking-Design-System?node-id=0-1&t=YTR1ET4Qw1wG1cjz-1',
🧹 Nitpick comments (4)
libs/website/shared/ui/banner/banner.tsx (1)
13-13: Remove or address TODO comment.The TODO comment about social media names should either be addressed in this PR or tracked in a separate issue.
Would you like me to create a GitHub issue to track this improvement?
libs/website/shared/ui/banner/banner.stories.tsx (2)
45-45: Consider making MLH URL configurable.The MLH link URL contains tracking parameters. Consider moving this to a configuration constant to make it easier to update across environments.
40-51: Add more story variants.Consider adding additional stories to showcase different banner configurations:
- Multiple banners
- Different sizes/layouts
- Error states (e.g., missing media)
libs/website/shared/ui/navigation/navbar/ui/navbar.presenter.tsx (1)
15-17: Consider combining imports for better organization.The React and Banner imports could be grouped with related imports for better code organization.
-import React, { useState } from 'react' -import { ClientOnly } from 'remix-utils/client-only' -import { Banner } from '../../../banner' +import React, { useState } from 'react' +import { ClientOnly } from 'remix-utils/client-only' +import { Banner } from '../../../banner'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
libs/shared/assets/logos/sponsors/mlh-banner.svgis excluded by!**/*.svg
📒 Files selected for processing (8)
libs/website/shared/ui/banner/banner.stories.tsx(1 hunks)libs/website/shared/ui/banner/banner.tsx(1 hunks)libs/website/shared/ui/banner/index.ts(1 hunks)libs/website/shared/ui/navigation/navbar/constants/navbar.constants.ts(3 hunks)libs/website/shared/ui/navigation/navbar/ui/navbar.container.tsx(3 hunks)libs/website/shared/ui/navigation/navbar/ui/navbar.presenter.stories.tsx(3 hunks)libs/website/shared/ui/navigation/navbar/ui/navbar.presenter.tsx(4 hunks)libs/website/shared/ui/socials/social.stories.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/website/shared/ui/banner/index.ts
🔇 Additional comments (10)
libs/website/shared/ui/navigation/navbar/ui/navbar.container.tsx (1)
14-18: LGTM! Interface and prop handling look good.The banner property is well-structured and follows existing patterns. Props are correctly typed and propagated to the presenter component.
Also applies to: 30-30, 40-40
libs/website/shared/ui/banner/banner.tsx (1)
1-29: Add tests for the Banner component.As mentioned in the PR description, tests are missing. Please add tests to cover:
- Rendering with different banner configurations
- Link functionality
- Accessibility features
Would you like me to help generate test cases for this component?
libs/website/shared/ui/socials/social.stories.tsx (1)
54-54: LGTM! Good accessibility improvement.Adding the name property to social media objects improves accessibility and maintains consistency with the banner implementation.
Also applies to: 59-59, 64-64, 69-69, 74-74, 79-79, 84-84, 89-89
libs/website/shared/ui/navigation/navbar/constants/navbar.constants.ts (3)
13-13: LGTM!The MLH banner import is correctly placed with other logo imports.
22-31: LGTM! Banner configuration follows best practices.The banner configuration:
- Uses proper UTM parameters for MLH tracking
- Includes descriptive alt text for accessibility
- Follows the same structure as socials for consistency
105-105: LGTM!The MLH_BANNER is correctly exported in NAVBAR_CONSTANTS.
libs/website/shared/ui/navigation/navbar/ui/navbar.presenter.tsx (2)
33-37: LGTM! Interface update follows TypeScript best practices.The banner property in NavbarProps follows the same structure as socials for consistency.
69-71: Verify banner placement and responsiveness.The banner placement between logo and navigation menu:
- Ensure it doesn't break layout on different screen sizes
- Verify it follows MLH placement guidelines
- Confirm the hidden/flex classes work as expected
✅ Verification successful
Banner implementation follows MLH guidelines and responsive design best practices
The banner implementation is correctly placed and follows responsive design patterns:
- Properly positioned between logo and navigation
- Correctly hidden on mobile and shown on desktop/tablet screens
- Uses official MLH season URL and proper accessibility attributes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for MLH banner placement guidelines in documentation rg -i "mlh.*banner.*placement|banner.*placement.*guidelines" README.md docs/ # Find other responsive design patterns in the codebase rg "hidden md:" --type ts --type tsxLength of output: 191
Script:
#!/bin/bash # Search for documentation files and MLH/banner content fd -e md -e mdx | xargs rg -i "mlh|banner" # Find the Banner component implementation ast-grep --pattern 'const Banner = $_' # Search for responsive patterns with correct type flags rg "hidden.*md:" -t typescript -t typescriptreact # Find all Banner component usage rg "Banner" -t typescript -t typescriptreactLength of output: 1566
Script:
#!/bin/bash # Find and inspect navbar-related files fd navbar libs/ | xargs cat # Find Banner component implementation fd -e ts -e tsx | xargs rg "Banner.*=.*{" -A 5 # Search for responsive patterns in the specific directory fd -e ts -e tsx . libs/website/shared/ui/navigation/ | xargs rg "hidden|md:" # Find all Banner imports and usage fd -e ts -e tsx | xargs rg "import.*Banner|<Banner"Length of output: 20595
libs/website/shared/ui/navigation/navbar/ui/navbar.presenter.stories.tsx (2)
46-55: LGTM! Banner configuration matches constants.The banner configuration in stories matches the one in navbar.constants.ts.
137-137: LGTM! Story args updated correctly.The banner is correctly added to the Default story args.
| <Link | ||
| target="_blank" | ||
| key={link} | ||
| to={link} | ||
| aria-label={name} | ||
| > |
There was a problem hiding this comment.
Add security attributes for external links.
When using target="_blank", it's important to include rel="noopener noreferrer" to prevent potential security vulnerabilities.
<Link
target="_blank"
key={link}
to={link}
aria-label={name}
+ rel="noopener noreferrer"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Link | |
| target="_blank" | |
| key={link} | |
| to={link} | |
| aria-label={name} | |
| > | |
| <Link | |
| target="_blank" | |
| key={link} | |
| to={link} | |
| aria-label={name} | |
| rel="noopener noreferrer" | |
| > |
There was a problem hiding this comment.
CodeRabbit, can you link this PR with #242 (by making sure that the issues closes when merging this pull request by linking this branch to the pull request)? THanks
There was a problem hiding this comment.
@JeremyFriesenGitHub I apologize, but I don't have the necessary permissions to modify the PR directly. However, you can easily link this PR to issue #242 by:
- Adding "Closes feat(ui/website): add MLH banner to landing page #242" or "Fixes feat(ui/website): add MLH banner to landing page #242" to your PR description
- Or adding it to any of your commit messages
Once you do this, the issue will automatically close when the PR is merged. Would you like me to help you with anything else?
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# First, let's verify the issue exists and get its details
gh issue view 242
# Get current PR description
gh pr view 243 --json body -q .body
# Update PR body to include the closing keyword
# Note: We'll append "Closes #242" to the existing description
gh pr edit 243 --body "$(gh pr view 243 --json body -q .body)
Closes #242"
Length of output: 6549
⛔ Permission issue
There was a permission issue. Please ask the repository administrator to refresh CodeRabbit's permissions at https://github.com/apps/coderabbitai/installations/54504200.
JeremyFriesenGitHub
force-pushed
the
jeremy/feat/242-add-MLH-banner-to-landing-page
branch
from
5248c33 to
fd95219
Compare
HasithDeAlwis
force-pushed
the
jeremy/feat/242-add-MLH-banner-to-landing-page
branch
from
fd95219 to
569a67f
Compare
anakafeel
force-pushed
the
jeremy/feat/242-add-MLH-banner-to-landing-page
branch
from
569a67f to
333cb89
Compare
HasithDeAlwis
requested review from
HasithDeAlwis and
MFarabi619
as code owners
anakafeel
force-pushed
the
jeremy/feat/242-add-MLH-banner-to-landing-page
branch
from
333cb89 to
afd209a
Compare
anakafeel
force-pushed
the
jeremy/feat/242-add-MLH-banner-to-landing-page
branch
from
afd209a to
b0a6e25
Compare
|
🎉 This PR is included in version 2.10.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Checklist
Additional Comments
I went ahead to start the scaffolding for this feature. Since I'm unsure about required conventions and formatting of the banner, I will leave this to the core team to complete.
Essentially, I mirrored the socials component code to create the banner. However, I've run into issues with the mapping function and I don't know why I'm getting errors, since it runs in Storybook (see screenshots below).
Also, the site that I used to get the banner and the SVG is here.
Tests need to be added as well
Screenshots (if applicable)
Summary by CodeRabbit
Release Notes
New Features
Bannercomponent to display promotional bannersImprovements
Documentation