-
Notifications
You must be signed in to change notification settings - Fork 73
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: add StudioSearch component #14348
Conversation
frontend/libs/studio-components/src/components/StudioSearch/index.ts
Outdated
Show resolved
Hide resolved
Vi har et søkefelt på dashbordet også. Hvis det ikke er for mye jobb, kan du legge den til der i tillegg? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14348 +/- ##
=======================================
Coverage 95.66% 95.66%
=======================================
Files 1871 1873 +2
Lines 24287 24297 +10
Branches 2788 2789 +1
=======================================
+ Hits 23233 23244 +11
Misses 797 797
+ Partials 257 256 -1 ☔ View full report in Codecov by Sentry. |
frontend/libs/studio-components/src/components/StudioSearch/StudioSearch.test.tsx
Outdated
Show resolved
Hide resolved
Warning Rate limit exceeded@standeren has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 25 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 selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive update to the search functionality across multiple frontend components. The changes primarily focus on replacing existing search implementations with a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
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.
Test
Dashboard
Label er over søkefeltet i PR-beskrivelsen, men ser det nå står til venstre:
Språk
Label kommer helt inntil tekstfeltet når det er fokusert. Kan du gjøre avstanden litt større?
Bibliotek
Her har label havnet til venstre for tekstfelt, og ser i PR-beskrivelsen at den skal være over feltet.
Edit: fjernet en bolk som angikk feil konfig av .env hos meg 😅
8efefb3
to
3cfe28c
Compare
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/libs/studio-components/src/components/StudioSearch/StudioSearch.tsx (1)
13-22
: Consider adding aria-label when no visible label is present.While the implementation looks good, consider adding an aria-label when
showLabel
is false to maintain accessibility.return ( <div> {showLabel && ( <Label htmlFor={searchId} spacing> {label} </Label> )} - <Search {...rest} id={searchId} size={size} ref={ref} /> + <Search + {...rest} + id={searchId} + size={size} + ref={ref} + aria-label={!showLabel && label ? label : undefined} + /> </div> );frontend/libs/studio-components/src/components/StudioSearch/StudioSearch.test.tsx (1)
22-27
: Add test for aria-label when no visible label is present.To ensure accessibility is maintained, add a test case for the aria-label when
showLabel
is false.+ it('should have aria-label when visible label is not shown', () => { + const label = 'Search for something'; + renderTestSearch({ label, showLabel: false }); + const search = screen.getByRole('searchbox', { name: label }); + expect(search).toHaveAttribute('aria-label', label); + });frontend/testing/playwright/pages/DashboardPage.ts (1)
96-96
: Consider adding a name to the searchbox role selector.While using
getByRole('searchbox')
follows accessibility best practices, adding the expected label text would make the test more specific and robust.- await this.page.getByRole('searchbox').fill(word); + await this.page.getByRole('searchbox', { name: this.textMock('dashboard.search') }).fill(word);frontend/dashboard/pages/Dashboard/Dashboard.module.css (1)
6-7
: Consider enhancing responsive layoutWhile the grid layout works, consider these improvements for better responsiveness:
- Add media queries for narrow viewports
- Define minimum column widths
- Consider fallback for older browsers
Example improvement:
.topBar { display: grid; - grid-template-columns: auto auto; + grid-template-columns: minmax(200px, 1fr) minmax(auto, max-content); justify-content: space-between; align-items: center; margin-bottom: var(--fds-spacing-10); + gap: var(--fds-spacing-4); + @media (max-width: 768px) { + grid-template-columns: 1fr; + gap: var(--fds-spacing-6); + } }Also applies to: 55-55
🛑 Comments failed to post (1)
frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListsActionsBar/CodeListsActionsBar.tsx (1)
38-41: 💡 Codebase verification
Missing required controlled component props
The StudioSearch component requires controlled component implementation as evidenced by other usages in the codebase (e.g., in TextEditor.tsx and Dashboard.tsx). The current implementation should include:
value
prop for controlled stateonChange
handler for input changes🔗 Analysis chain
Missing search functionality props.
The StudioSearch component appears to be missing essential props for search functionality:
onChange
handler for search inputvalue
prop for controlled inputplaceholder
for better UX when label is visible🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if search functionality is implemented elsewhere rg -A 5 "StudioSearch.*onChange" .Length of output: 36
Script:
#!/bin/bash # Find StudioSearch component definition fd StudioSearchLength of output: 237
Script:
#!/bin/bash # Look for any StudioSearch usage patterns rg "StudioSearch" -A 3Length of output: 6543
Script:
#!/bin/bash # Find component imports rg "import.*StudioSearch"Length of output: 711
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.
Ser bra ut 🥳
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
🧹 Nitpick comments (1)
frontend/libs/studio-components/src/components/StudioSearch/StudioSearch.tsx (1)
14-14
: Consider enhancing className handling.The
className
prop is directly passed to the wrapper div. Consider using a utility likeclsx
orclassnames
to combine multiple class names and ensure robust handling of undefined or null values.- <div className={className}> + <div className={clsx(className)}>Don't forget to add the import:
import clsx from 'clsx';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/dashboard/pages/Dashboard/Dashboard.module.css
(0 hunks)frontend/dashboard/pages/Dashboard/Dashboard.tsx
(2 hunks)frontend/libs/studio-components/src/components/StudioSearch/StudioSearch.tsx
(1 hunks)frontend/packages/text-editor/src/TextEditor.tsx
(3 hunks)
💤 Files with no reviewable changes (1)
- frontend/dashboard/pages/Dashboard/Dashboard.module.css
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/dashboard/pages/Dashboard/Dashboard.tsx
- frontend/packages/text-editor/src/TextEditor.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: Typechecking and linting
- GitHub Check: CodeQL
🔇 Additional comments (4)
frontend/libs/studio-components/src/components/StudioSearch/StudioSearch.tsx (4)
1-5
: LGTM! Clean imports and type definitions.The imports are well-organized, and the
WithoutAsChild
utility is used appropriately to define the component's props type.
7-24
: LGTM! Well-structured component implementation.The component follows React best practices with proper ref forwarding, ID generation, and label accessibility.
26-28
: LGTM! Proper component naming and export.The
displayName
is set correctly for debugging purposes, and the export follows the standard pattern.
1-28
: Verify integration points across the application.Since this component replaces existing search implementations in multiple places (dashboard, text editor, code list page), let's verify the integration points:
✅ Verification successful
Integration verification successful
The StudioSearch component is properly integrated across all intended locations (dashboard, text editor, code list page) with consistent implementation patterns. No direct usage of the base Search component remains in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify StudioSearch component usage across the application # Check import statements echo "Checking StudioSearch imports:" rg -l "import.*StudioSearch" --type ts --type tsx # Check component usage echo "\nChecking StudioSearch component usage:" rg "<StudioSearch" --type tsx -A 3 # Check for any remaining old search implementations echo "\nChecking for potential remaining old implementations:" rg -l "import.*Search.*from.*@digdir/designsystemet-react" --type ts --type tsxLength of output: 537
Script:
#!/bin/bash # Description: Verify StudioSearch component usage with corrected syntax echo "Checking StudioSearch imports:" rg "import.*StudioSearch" -g "*.{ts,tsx}" echo -e "\nChecking StudioSearch component usage:" rg "<StudioSearch" -g "*.tsx" -A 3 echo -e "\nChecking for potential remaining old Search implementations:" rg "import.*Search.*from.*@digdir/designsystemet-react" -g "*.{ts,tsx}"Length of output: 3428
Description
StudioSearch
component.Dashboard:
nt on dashboard.
Text-editor:
CodeLists in content library:
Related Issue(s)
Verification
Summary by CodeRabbit
Release Notes
New Features
StudioSearch
component to enhance search functionality across multiple pages.Improvements
Refactoring
StudioSearch
component.