fix(tp): fix career partner banner UI on tablet and mobile #993
Conversation
WalkthroughThe pull request includes modifications to the styling and functionality of navigation components and the Career Partner banner within the application. Key changes involve updating CSS styles for responsiveness, introducing new classes for active states in navigation items, and altering JSX structure for better alignment with design specifications. Conditional rendering has been implemented to enhance navigation based on user context, specifically for jobseekers. These adjustments aim to improve the user interface across various screen sizes. Changes
Assessment against linked 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: 3
🧹 Outside diff range and nitpick comments (3)
apps/redi-talent-pool/src/components/organisms/CareerPartnerBanner.scss (1)
Line range hint 72-83: Remove unnecessary line-height percentage comments.
The implementation looks good, but the percentage comments (/* 156.25% / and / 148.148% */) don't add value and should be removed for cleaner code.
font-size: 16px;
- line-height: 25px; /* 156.25% */
+ line-height: 25px;
@include mobile() {
font-size: 13.5px;
- line-height: 20px; /* 148.148% */
+ line-height: 20px;
}apps/redi-talent-pool/src/components/molecules/TpMainNavItem.scss (2)
11-23: Consider using a standard breakpoint variable
While the implementation is correct, the hardcoded breakpoint value (1191px) should be replaced with a standard breakpoint variable for better maintainability.
Consider using Bulma's built-in breakpoint variables since you're already importing Bulma:
- @media screen and (max-width: 1191px) {
+ @media screen and (max-width: $desktop - 1px) {25-39: Clean up horizontal active bar implementation
Two suggestions for improvement:
- Use the same breakpoint variable as suggested earlier for consistency
- Remove the unnecessary
content: '';declaration on line 31 as it's not needed for block elements
Apply these changes:
- @media screen and (max-width: 1191px) {
+ @media screen and (max-width: $desktop - 1px) {
display: block;
- content: '';
position: absolute;📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- apps/redi-talent-pool/src/components/molecules/TpMainNavItem.scss (1 hunks)
- apps/redi-talent-pool/src/components/molecules/TpMainNavItem.tsx (1 hunks)
- apps/redi-talent-pool/src/components/organisms/CareerPartnerBanner.scss (2 hunks)
- apps/redi-talent-pool/src/components/organisms/CareerPartnerBanner.tsx (1 hunks)
- apps/redi-talent-pool/src/components/templates/LoggedIn.tsx (1 hunks)
🔇 Additional comments (11)
apps/redi-talent-pool/src/components/organisms/CareerPartnerBanner.tsx (1)
Line range hint 1-53: Verify responsive layout implementation.
The structural changes look good, but let's ensure all responsive styles are properly implemented in the associated SCSS file.
✅ Verification successful
Responsive styles are properly implemented
The SCSS file includes comprehensive responsive styles using Bulma's @include mobile() mixins. The implementation covers:
- Adjusted margins and padding for mobile view
- Proper spacing adjustments for separators and badges
- Responsive font sizes and line heights for the bottom text
- Appropriate padding modifications for various elements in mobile view
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if responsive styles are implemented in the SCSS file
rg -A 3 "@media" "apps/redi-talent-pool/src/components/organisms/CareerPartnerBanner.scss"
Length of output: 88
Script:
#!/bin/bash
# Let's first check if the SCSS file exists and see its content
cat apps/redi-talent-pool/src/components/organisms/CareerPartnerBanner.scss
Length of output: 1655
apps/redi-talent-pool/src/components/organisms/CareerPartnerBanner.scss (4)
1-15: LGTM! Responsive design improvements look good.
The base structure uses appropriate flex layout and the mobile adjustments create better spacing and containment for smaller screens. The max-width of 550px is a good choice to maintain readability on tablets.
34-71: LGTM! Well-structured responsive typography and spacing.
The badge and metadata sections have appropriate responsive adjustments and consistent typography settings. The mobile-specific margin and padding adjustments align well with the PR objectives for better tablet/mobile layouts.
24-32: 🛠️ Refactor suggestion
Consider simplifying the separator implementation.
While the current implementation works, it could be simplified using a single border property.
&__separator {
- width: 1px;
height: 2rem;
- border-left: 1px solid black;
+ border: 1px solid black;
margin: 0 24px;
@include mobile() {
margin: 0 16px;
}
}Likely invalid or redundant comment.
Line range hint 1-83: Verify the tablet-specific layout requirements.
While the mobile responsive adjustments look good, the PR objectives mention specific requirements about the active bar visibility in tablet layout. Please ensure these tablet-specific requirements are addressed, possibly through a tablet-specific media query.
apps/redi-talent-pool/src/components/molecules/TpMainNavItem.tsx (2)
Line range hint 1-99: Clean and well-structured implementation!
The component follows React best practices with:
- Proper use of TypeScript interfaces
- Callback optimization with useCallback
- Clean separation of concerns
- Correct ref forwarding implementation
57-62: Verify responsive behavior of active bars.
The implementation adds two active bar elements that are conditionally rendered together. According to the PR objectives, the active bar should only be visible in desktop layout.
Let's verify the responsive behavior:
Consider adding comments to clarify the purpose of each active bar element:
{isActive && (
<>
+ {/* Vertical active bar for desktop view */}
<div className="tp-main-nav-item__active-bar"></div>
+ {/* Horizontal active bar for tablet/mobile view */}
<div className="tp-main-nav-item__active-bar--horizontal"></div>
</>
)}✅ Verification successful
Active bars implementation correctly handles responsive behavior
The SCSS implementation properly manages the visibility of active bars across different layouts:
- Vertical bar (
tp-main-nav-item__active-bar) is visible only on desktop and hidden below 1191px - Horizontal bar (
tp-main-nav-item__active-bar--horizontal) is hidden by default and shown below 1191px
The implementation aligns with the PR objectives for responsive behavior. The code comments in the SCSS file also provide clear documentation of the purpose of each bar.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the active bars have proper responsive styling
# Expected: Find media queries controlling visibility of active bars
# Search for related SCSS rules
rg -t scss "tp-main-nav-item__active-bar"
# Search for related media queries
rg -t scss "@media.*tp-main-nav-item__active-bar"
Length of output: 151
Script:
#!/bin/bash
# Let's try searching without file type restriction since scss might not be recognized
# Search for SCSS/CSS files first
fd -e scss -e css
# Then search for the class names in all files
rg "tp-main-nav-item__active-bar"
# Also search for any media query related styling
rg "@media.*tp-main-nav-item"
Length of output: 5735
Script:
#!/bin/bash
# Let's check the content of the SCSS file for the component
cat apps/redi-talent-pool/src/components/molecules/TpMainNavItem.scss
# Also search for any responsive styles in this file
rg -A 5 -B 5 "@media" apps/redi-talent-pool/src/components/molecules/TpMainNavItem.scss
Length of output: 2957
apps/redi-talent-pool/src/components/molecules/TpMainNavItem.scss (1)
7-9: LGTM: Container positioning improvements
The changes to justify-content and positioning create a proper layout foundation for both the centered content and absolutely positioned active indicators.
apps/redi-talent-pool/src/components/templates/LoggedIn.tsx (3)
70-87: Clean implementation of conditional profile navigation.
The conditional rendering based on user type improves the navigation experience by directing jobseekers to their specific profile page while maintaining the default behavior for other users.
Let's verify the navigation behavior across different user types:
✅ Verification successful
Navigation implementation is correctly integrated across the codebase
The verification shows that:
- The conditional profile navigation is properly implemented in
LoggedIn.tsx - The redirection logic in
Me.tsxcorrectly handles jobseeker profiles by redirecting to/app/jobseeker-profile/${id} TpMainNavItemusage is consistent throughout the application with proper page type definitions- No conflicting implementations of profile navigation were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify navigation implementation consistency
# Look for any other profile-related routing logic that might need updates
# Search for profile route definitions
echo "Checking profile route definitions..."
rg -l "jobseeker-profile|/app/me" --type tsx
# Search for potential profile navigation logic
echo "Checking profile navigation logic..."
ast-grep --pattern 'to={$_("/app/me")}'
Length of output: 284
Script:
#!/bin/bash
# Let's try with correct file extensions and more specific patterns
# Search for profile route definitions and navigation logic
echo "Checking profile routes and navigation..."
rg -l "jobseeker-profile|/app/me" --type ts --type tsx --type jsx
# Search for TpMainNavItem usage
echo "Checking TpMainNavItem usage..."
rg "TpMainNavItem" -A 3 --type ts --type tsx --type jsx
# Look for other profile-related routing
echo "Checking other profile routing..."
rg "to=.*(/app/me|/app/jobseeker-profile)" --type ts --type tsx --type jsx
Length of output: 527
Script:
#!/bin/bash
# Let's try without file type restrictions since the environment might not recognize tsx/jsx
# Search for profile route definitions and navigation logic
echo "Checking profile routes and navigation..."
rg -l "jobseeker-profile|/app/me"
# Search for TpMainNavItem usage
echo "Checking TpMainNavItem usage..."
rg "TpMainNavItem" -A 3
# Look for other profile-related routing
echo "Checking other profile routing..."
rg "to=.*(/app/me|/app/jobseeker-profile)"
Length of output: 11969
70-87: Verify tablet/mobile UI requirements.
While the navigation logic is sound, the PR objectives mention tablet/mobile UI fixes. This component might need additional styling updates to fully address the UI requirements.
Let's check for related style definitions:
✅ Verification successful
Let me analyze the styling implementation with these commands first.
Mobile/tablet UI requirements are properly implemented
The navigation component has comprehensive responsive styling implementations:
- The side menu (
tp-side-menu) transforms into a fixed bottom navigation on mobile/tablet using@include touch() TpMainNavItemhas specific mobile/tablet adaptations:- Active state indicator switches from vertical (right side) to horizontal (top) bar below 1191px
- Icon sizes and spacing are consistently maintained across devices
- The navbar has proper mobile adaptations including padding adjustments and a mobile menu overlay
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify responsive styling implementation
# Look for responsive styles that affect the navigation
echo "Checking for responsive styles..."
fd "LoggedIn.scss|TpMainNavItem.scss" --type scss --exec cat {}
Length of output: 395
Script:
#!/bin/bash
# Let's try a different approach to find style files and check responsive styles
# First, let's find the exact location of style files related to navigation
# Look for any CSS/SCSS files related to LoggedIn or TpMainNavItem
fd -e css -e scss . | grep -i "logged\|nav"
# Also search for responsive style patterns in TSX files
rg -i "@media|breakpoint|mobile|tablet" "LoggedIn.tsx|TpMainNavItem.tsx" --type tsx
# Search for styled-components or other styling approaches
ast-grep --pattern 'styled.$_'
Length of output: 551
Script:
#!/bin/bash
# Let's examine the content of the relevant style files we found
echo "=== LoggedIn.scss content ==="
cat "apps/redi-talent-pool/src/components/templates/LoggedIn.scss"
echo -e "\n=== TpMainNavItem.scss content ==="
cat "apps/redi-talent-pool/src/components/molecules/TpMainNavItem.scss"
echo -e "\n=== Navbar.scss content ==="
cat "apps/redi-talent-pool/src/components/organisms/Navbar.scss"
Length of output: 4860
75-78: Verify active state behavior for deep routes.
The current active state check is exact (===). Consider if this should handle nested routes or partial matches for better UX.
Let's check for potential edge cases:
✅ Verification successful
The exact match for jobseeker profile route is correct - no changes needed
Looking at the routes configuration, /app/jobseeker-profile/:tpJobseekerProfileId is defined with exact: true and there are no nested routes under this path. The current exact match implementation in LoggedIn.tsx aligns with the routing setup. The CV builder uses includes() because it has nested routes (/app/cv-builder and /app/cv-builder/:id), but the jobseeker profile doesn't have this requirement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for nested routes that might break active state
# Look for routes that extend the base jobseeker profile path
echo "Checking for nested profile routes..."
rg "/app/jobseeker-profile/[^\"']*/" --type tsx
Length of output: 395
Script:
#!/bin/bash
# Let's try a different approach to find nested routes and route patterns
# First, let's check the routing setup in the codebase
rg -A 5 "Route.*path.*jobseeker-profile" --type ts --type tsx
# Also check for any navigation or link components using this path
rg -A 2 "to=.*jobseeker-profile" --type ts --type tsx
# And check the component itself for any route-related logic
fd "LoggedIn.tsx" --type file --exec cat "{}"
Length of output: 9686
Script:
#!/bin/bash
# Let's check for any nested routes under CV builder since we see it uses includes()
rg -l "/app/cv-builder" --type-add 'tsx:*.{tsx,ts}' --type tsx
# Also check for route definitions
fd "router|routes" -e tsx -e ts --exec cat "{}"
Length of output: 10947
| <div className="career-partner-banner-top__badge"> | ||
| <Icon | ||
| icon="careerPartnerBadge" | ||
| size="large" | ||
| className="career-partner-badge-icon" | ||
| className="career-partner-banner-top__badge-icon" | ||
| /> | ||
| <Heading size="smaller">Career Partner</Heading> | ||
| <Heading | ||
| size="smaller" | ||
| className="career-partner-banner-top__badge-text" | ||
| > | ||
| Career | ||
| <br /> | ||
| Partner | ||
| </Heading> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving heading accessibility for screen readers.
While the visual layout with <br /> tags works well, screen readers might pause awkwardly between "Career" and "Partner". Consider using CSS for visual formatting while maintaining semantic meaning.
- <Heading
- size="smaller"
- className="career-partner-banner-top__badge-text"
- >
- Career
- <br />
- Partner
- </Heading>
+ <Heading
+ size="smaller"
+ className="career-partner-banner-top__badge-text"
+ aria-label="Career Partner"
+ >
+ <span className="career-partner-banner-top__badge-text-line">Career</span>
+ <span className="career-partner-banner-top__badge-text-line">Partner</span>
+ </Heading>Add to your SCSS:
.career-partner-banner-top__badge-text-line {
display: block;
}
What Github issue does this PR relate to? Insert link.
Closes #964
What should the reviewer know?
In this PR, we:
Screenshots
Tablet view:
Mobile view:
Jobseeker view:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style