-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat(web): Add search input to organization standalone theme mobile frontpage #17095
feat(web): Add search input to organization standalone theme mobile frontpage #17095
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17095 +/- ##
==========================================
- Coverage 35.66% 35.66% -0.01%
==========================================
Files 6947 6947
Lines 147512 147517 +5
Branches 41938 41942 +4
==========================================
Hits 52615 52615
- Misses 94897 94902 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 12 Total Test Services: 0 Failed, 11 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/web/layouts/organization/standalone/components/Header.tsx (1)
61-106
: Consider extracting image styles to a separate constantThe image styling logic contains multiple conditional properties that could be extracted to improve readability and maintainability.
const getImageStyles = ( imagePadding: string, imageObjectFit: 'contain' | 'cover', imageObjectPosition: 'left' | 'center' | 'right', imageIsFullHeight: boolean, ) => ({ padding: imagePadding, objectFit: imageObjectFit, objectPosition: imageObjectPosition, height: imageIsFullHeight ? '100%' : undefined, })apps/web/layouts/organization/standalone/standalone.tsx (1)
70-70
: Consider consistent null handling between header and navigation propsWhile the header props use optional chaining with fallback (
??
), the navigation props directly access the slug. Consider using the same pattern for consistency.- organizationSlug: organizationPage.organization?.slug, + organizationSlug: organizationPage.organization?.slug ?? '',
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/layouts/organization/standalone/components/Header.tsx
(4 hunks)apps/web/layouts/organization/standalone/standalone.tsx
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/web/layouts/organization/standalone/standalone.tsx (1)
Pattern apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/layouts/organization/standalone/components/Header.tsx (1)
Pattern apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (4)
apps/web/layouts/organization/standalone/components/Header.tsx (3)
5-15
: LGTM: Clean import organization
The new imports are well-organized, grouped by their source packages, and follow NextJS best practices.
33-33
: LGTM: Type-safe prop addition
The optional organizationSlug
property is properly typed as string, maintaining type safety.
107-118
: Verify mobile-only search input implementation
The search input is correctly implemented for mobile views using the Hidden
component, but we should verify the behavior across different screen sizes.
✅ Verification successful
The search results show consistent usage of the Hidden above="md"
component across the codebase for mobile-specific implementations. Let me verify the search input implementation further by checking for other instances and their screen size handling.
Based on the search results, I can verify that the search input implementation is consistent with the codebase's patterns. Let me check one final aspect to ensure the desktop view is properly handled.
Search input implementation is correctly handled for both mobile and desktop views
The implementation is properly structured with:
- Mobile view: Search input wrapped in
Hidden above="md"
in Header.tsx - Desktop view: Search input implemented in Navigation.tsx without visibility restrictions
- Consistent props and styling across both implementations
- Proper localization handling for placeholders
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent mobile-specific implementations across the codebase
# Look for other mobile-specific Hidden component usage patterns
rg -A 3 "Hidden above=\"md\""
# Check for any existing search input implementations that might conflict
ast-grep --pattern 'SearchInput organization={$_}'
Length of output: 12262
Script:
#!/bin/bash
# Check for other SearchInput implementations and their screen size handling
rg -A 5 "SearchInput"
# Look for complementary desktop-only search implementations
rg -A 3 "Hidden below=\"(lg|md)\""
Length of output: 65822
Script:
#!/bin/bash
# Check for complementary desktop search implementation in the same file
rg -A 5 "Hidden below=\"lg\"" "apps/web/layouts/organization/standalone/components/Header.tsx"
# Check for other search input implementations in the navigation
rg -A 5 "SearchInput" "apps/web/layouts/organization/standalone/components/Navigation.tsx"
Length of output: 628
apps/web/layouts/organization/standalone/standalone.tsx (1)
70-70
: Verify organization slug usage across the application
Let's ensure the organization slug is consistently handled across the application, especially in search-related components.
✅ Verification successful
Organization slug usage is consistent across the application
Based on the search results, the organization slug is consistently handled across the application:
- It's properly used in URL generation and routing via
linkResolver
- It's correctly passed to search components and filters
- The implementation in the standalone layout matches the pattern used in other components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent organization slug handling
# Check for other instances of organization slug usage in search components
rg -A 3 "organization={" --type=typescript --type=tsx
# Look for potential missing organization slug propagation
ast-grep --pattern 'SearchInput $$$'
Length of output: 186
Script:
#!/bin/bash
# Search for organization slug usage patterns
rg "organization[.:]slug" --type=typescript --type=tsx -A 2
# Search for SearchInput component usage
rg "SearchInput" --type=typescript --type=tsx -A 2
# Look for organization prop patterns in components
rg "organization: ?{" --type=typescript --type=tsx -A 2
rg "organization={" --type=typescript --type=tsx -A 2 --no-heading
Length of output: 497
Script:
#!/bin/bash
# Search for organization slug usage patterns with proper escaping
rg "organization\.slug" .
# Search for SearchInput component usage
rg "SearchInput" .
# Look for organization prop patterns in components with proper escaping
rg "organization\s*=" .
Length of output: 23722
Add search input to organization standalone theme mobile frontpage
Screenshots / Gifs
Before
After
Checklist:
Summary by CodeRabbit
New Features
organizationSlug
property.Bug Fixes
Documentation
organizationSlug
property.