Communication: Add FAQ search bar
#9423
Conversation
WalkthroughThe changes in this pull request involve updates to the 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
Documentation and Community
|
github-actions
bot
added
the
client
cremertim
changed the title
Communicaiton Implement FAQ search bar
|
github-actions
bot
added
the
deployment-error
cremertim
removed
the
deployment-error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range comments (1)
src/main/webapp/app/overview/course-faq/course-faq.component.ts (1)
Line range hint
1-99: Overall implementation of search feature is on track, but needs completion.The changes made to the CourseFaqComponent are well-structured and align with the PR objectives of implementing a search bar for the FAQ section. The component now includes the necessary imports, properties, and methods to support search functionality. However, the actual filtering of FAQs based on the search value is not yet implemented.
To complete the feature, consider updating the applyFilters method or creating a new method to filter the FAQs based on the searchValue. This will ensure that the FAQ list updates dynamically as the user types in the search bar, providing a responsive and user-friendly search experience.
Additionally, ensure that the search functionality is thoroughly tested, including edge cases and performance considerations, as outlined in the PR objectives.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (5)
- src/main/webapp/app/overview/course-faq/course-faq.component.html (1 hunks)
- src/main/webapp/app/overview/course-faq/course-faq.component.ts (3 hunks)
- src/main/webapp/app/shared/search/search.component.html (1 hunks)
- src/main/webapp/app/shared/search/search.component.scss (1 hunks)
- src/main/webapp/app/shared/search/search.component.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/overview/course-faq/course-faq.component.html (1)
Pattern
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.src/main/webapp/app/overview/course-faq/course-faq.component.ts (1)
src/main/webapp/app/shared/search/search.component.html (1)
Pattern
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.src/main/webapp/app/shared/search/search.component.ts (1)
🔇 Additional comments (10)
src/main/webapp/app/shared/search/search.component.scss (1)
1-4: 🛠️ Refactor suggestionConsider refactoring the
.mn-icon-fieldclass for better maintainability and responsiveness.While the class achieves its immediate goal, there are a few concerns:
The use of
!importantis generally discouraged as it can lead to specificity issues and make future style overrides difficult. Consider refactoring the CSS to avoid using!important.The fixed negative margin (
-3.5rem) might not be responsive across different screen sizes. Consider using relative units or a more flexible positioning approach.The
z-indexvalue of 10 is arbitrary and could potentially conflict with other elements. It's recommended to establish a z-index scale for the entire application to ensure consistent stacking behavior.Here's a suggestion for improvement:
.mn-icon-field { position: relative; display: inline-flex; align-items: center; &::before { content: '\1F50D'; // Unicode for a magnifying glass icon position: absolute; left: 0.5rem; pointer-events: none; } input { padding-left: 2rem; // Adjust based on your icon size } }This approach uses relative positioning and a pseudo-element for the icon, which is more flexible and doesn't rely on negative margins or
!important. Adjust the values as needed to fit your specific design requirements.To ensure this change doesn't affect other parts of the application, please run the following command:
This will help identify any other places where the class is used, allowing you to assess the impact of these changes.
✅ Verification successful
Verified usage of
.mn-icon-fieldacross multiple components.The
.mn-icon-fieldclass is utilized in the following locations:
src/main/webapp/app/shared/search/search.component.scsssrc/main/webapp/app/shared/search/search.component.htmlsrc/main/webapp/app/shared/search-filter/search-filter.component.scsssrc/main/webapp/app/shared/search-filter/search-filter.component.htmlPlease ensure that any refactoring of this class accounts for all its usages to maintain consistency and prevent unintended styling issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of mn-icon-field class rg --type html --type typescript 'mn-icon-field'Length of output: 83
Script:
#!/bin/bash # Attempting to search for 'mn-icon-field' in HTML and TypeScript files using correct type specifiers rg 'mn-icon-field' --type html --type ts --type tsxLength of output: 79
Script:
#!/bin/bash # Searching for 'mn-icon-field' in all files without specifying file types rg 'mn-icon-field'Length of output: 428
src/main/webapp/app/shared/search/search.component.html (2)
1-2: LGTM: Proper use of Angular reactive forms and Bootstrap classes.The form structure is well-defined using Angular's reactive forms, which is a good practice for managing form state and validation. The use of Bootstrap classes for styling is also appropriate.
1-19: Summary: Well-implemented search component aligning with PR objectives.The implemented search bar for the FAQ section is well-structured, using Angular best practices and reactive forms. It includes accessibility features and internationalization support, which aligns with the comprehensive approach outlined in the PR objectives.
A few minor suggestions were made to further enhance accessibility:
- Adding a visually hidden label for the search input.
- Including an aria-label for the search button.
These small improvements will make the search feature more inclusive without affecting the visual design.
The component appears ready for integration testing and performance review as per the PR checklist. Great job on implementing this feature!
src/main/webapp/app/shared/search/search.component.ts (2)
6-12: LGTM: Component decorator follows best practicesThe component decorator is well-structured and follows Angular's style guide. The use of a standalone component with explicit imports enhances modularity and maintainability.
1-31: Overall assessment: Good component structure with minor improvements neededThe SearchComponent is well-structured and follows many Angular best practices. It's a standalone component with clear separation of concerns. However, there are a few minor improvements needed:
- Correct the import of the Output decorator.
- Add proper type annotations and use the @output() decorator for the event emitter.
- Update the form control in the setSearchValue method to keep view and model in sync.
- Add access modifiers to methods for consistency.
After addressing these points, the component will be more robust and maintainable.
src/main/webapp/app/overview/course-faq/course-faq.component.html (2)
2-2: Improved layout using Bootstrap utility classesThe change from inline styles to Bootstrap utility classes (
d-flex justify-content-between) is a good practice. It enhances maintainability and consistency with the Bootstrap design system while providing a flexible container for the new search feature.
Line range hint
9-24: Correct usage of new Angular syntaxThe template correctly uses the new
@ifand@fordirectives instead of*ngIfand*ngFor, adhering to the provided coding guidelines. This adoption of the latest Angular syntax is commendable.src/main/webapp/app/overview/course-faq/course-faq.component.ts (3)
18-18: LGTM: SearchComponent import added.The import of SearchComponent aligns with the PR objective of implementing a search bar for the FAQ section. It follows the Angular style guide for imports.
26-26: LGTM: SearchComponent added to imports array.The addition of SearchComponent to the imports array is consistent with its usage in the component and follows Angular's best practices for component configuration.
42-43: LGTM: searchValue property added.The searchValue property is correctly named using camelCase and initialized as an empty string, following good practices for search input fields. This addition aligns with the PR objective of implementing a search feature for the FAQ section.
src/main/webapp/app/overview/course-faq/course-faq.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-faq/course-faq.component.ts
Outdated
Show resolved
Hide resolved
cremertim
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
cremertim
changed the title
Communicaiton Implement FAQ search barCommunicaition Implement FAQ search bar
cremertim
changed the title
Communicaition Implement FAQ search barCommunication Implement FAQ search bar
cremertim
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (1)
src/main/webapp/app/overview/course-faq/course-faq.component.html (1)
Line range hint
9-29: Update Angular structural directives for consistencyThe file uses a mix of new (@if, @for) and old (*ngIf, *ngFor) Angular syntax. According to the coding guidelines, @if and @for should always be used over the old style. Please update the remaining instances of *ngIf and *ngFor to use the new syntax.
For example, update:
<ul ngbDropdownMenu class="checkbox-menu text-nowrap pe-2" aria-labelledby="filter-dropdown-button"> @for (category of existingCategories; track category) { <li> <label class="d-flex align-items-center"> <input id="filter-{{ category.category }}" class="ms-2 form-check-input" (change)="toggleFilters(category.category!)" [checked]="activeFilters.has(category.category!)" type="checkbox" /> <jhi-custom-exercise-category-badge [category]="category" class="mt-2 ms-1" /> </label> </li> } </ul>to:
@if (hasCategories) { <ul ngbDropdownMenu class="checkbox-menu text-nowrap pe-2" aria-labelledby="filter-dropdown-button"> @for (category of existingCategories; track category) { <li> <label class="d-flex align-items-center"> <input id="filter-{{ category.category }}" class="ms-2 form-check-input" (change)="toggleFilters(category.category!)" [checked]="activeFilters.has(category.category!)" type="checkbox" /> <jhi-custom-exercise-category-badge [category]="category" class="mt-2 ms-1" /> </label> </li> } </ul> }This change will ensure consistency throughout the file and adherence to the coding guidelines.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (6)
- src/main/webapp/app/faq/faq.service.ts (1 hunks)
- src/main/webapp/app/overview/course-faq/course-faq.component.html (1 hunks)
- src/main/webapp/app/overview/course-faq/course-faq.component.ts (5 hunks)
- src/main/webapp/app/shared/search-filter/search-filter.component.ts (1 hunks)
- src/main/webapp/app/shared/shared.module.ts (0 hunks)
- src/main/webapp/app/shared/sidebar/sidebar.module.ts (2 hunks)
💤 Files with no reviewable changes (1)
- src/main/webapp/app/shared/shared.module.ts
🧰 Additional context used
📓 Path-based instructions (5)
src/main/webapp/app/faq/faq.service.ts (1)
src/main/webapp/app/overview/course-faq/course-faq.component.html (1)
Pattern
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.src/main/webapp/app/overview/course-faq/course-faq.component.ts (1)
src/main/webapp/app/shared/search-filter/search-filter.component.ts (1)
src/main/webapp/app/shared/sidebar/sidebar.module.ts (1)
🪛 Biome
src/main/webapp/app/faq/faq.service.ts
[error] 147-147: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
🔇 Additional comments (12)
src/main/webapp/app/shared/search-filter/search-filter.component.ts (2)
4-4: LGTM: Import of ArtemisSharedModuleThe import of
ArtemisSharedModuleis a good practice for code reuse and modularity. It aligns with the coding guidelines for promoting code reuse.
Line range hint
1-33: Overall assessment: Changes align with PR objectives and coding guidelinesThe modifications to make
SearchFilterComponentstandalone and importArtemisSharedModulealign well with the PR objectives of implementing a FAQ search bar. These changes enhance modularity and reusability, which can contribute to better performance and maintainability of the search functionality.The existing code adheres to the project's coding guidelines, including naming conventions, string quotes, and TypeScript best practices. The component's functionality for handling search events is preserved, supporting the PR's goal of adding search capabilities to the FAQ section.
To ensure these changes don't introduce any unintended side effects, please run the following verification script:
This script will help identify any potential issues related to the component's new standalone nature and its relationship with
ArtemisSharedModule.✅ Verification successful
Overall assessment: Changes align with PR objectives and coding guidelines
The verification confirms that
SearchFilterComponentis appropriately utilized across the codebase and that making it standalone does not introduce any issues.ArtemisSharedModuleis correctly exported, and there are no problematic circular dependencies detected. These changes enhance modularity and maintain the integrity of the search functionality as intended by the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of making SearchFilterComponent standalone # Test 1: Check for any remaining non-standalone usage of SearchFilterComponent echo "Checking for non-standalone usage of SearchFilterComponent:" rg --type typescript "SearchFilterComponent" -g "!**/search-filter.component.ts" # Test 2: Verify ArtemisSharedModule exports echo "Verifying ArtemisSharedModule exports:" rg --type typescript "export class ArtemisSharedModule" -A 10 -g "**/shared.module.ts" # Test 3: Check for any circular dependencies echo "Checking for potential circular dependencies:" rg --type typescript "import.*from.*search-filter" -g "!**/search-filter.component.ts"Length of output: 680
Script:
#!/bin/bash # Description: Re-verify the impact of making SearchFilterComponent standalone with corrected file patterns # Test 1: Check for any remaining non-standalone usage of SearchFilterComponent echo "Checking for non-standalone usage of SearchFilterComponent:" rg "SearchFilterComponent" -g "!**/search-filter.component.ts" -g "*.ts" -g "*.tsx" # Test 2: Verify ArtemisSharedModule exports echo "Verifying ArtemisSharedModule exports:" rg "export class ArtemisSharedModule" -A 10 -g "**/shared.module.ts" -g "*.ts" -g "*.tsx" # Test 3: Check for any circular dependencies echo "Checking for potential circular dependencies:" rg "import.*from.*search-filter" -g "!**/search-filter.component.ts" -g "*.ts" -g "*.tsx"Length of output: 7036
src/main/webapp/app/overview/course-faq/course-faq.component.html (1)
2-2: Improved layout with Bootstrap utility classesThe use of Bootstrap utility classes
d-flex justify-content-betweenfor layout management is a good practice. It improves the responsiveness and maintainability of the component.src/main/webapp/app/shared/sidebar/sidebar.module.ts (2)
19-19: LGTM: Import statement is correct.The import statement for
SearchFilterComponentfollows the coding guidelines and is correctly placed at the end of the import list.
32-32: Verify standalone component setup and consider usingimportProvidersFrom.The addition of
SearchFilterComponentto theimportsarray is correct for using standalone components in NgModules. However, to improve code organization and take full advantage of standalone components, consider the following:
- Verify that
SearchFilterComponentis indeed set up as a standalone component in its definition file.- If you're using Angular 14 or later, consider using
importProvidersFromfor a more explicit import of standalone components:import { importProvidersFrom } from '@angular/core'; @NgModule({ imports: [ // ... other imports importProvidersFrom(SearchFilterComponent), ], // ... rest of the module definition }) export class ArtemisSidebarModule {}This approach can help with better tree-shaking and make the intent of importing a standalone component more clear.
To verify the standalone component setup, please run the following script:
✅ Verification successful
Standalone Component Verification Successful
The
SearchFilterComponentis correctly set up as a standalone component.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify SearchFilterComponent is set up as a standalone component # Test: Check if SearchFilterComponent is decorated with standalone: true ast-grep --lang typescript --pattern 'class SearchFilterComponent { $$$ } @Component({ $$$ standalone: true, $$$ })' app/shared/search-filter/search-filter.component.ts # If the above command doesn't return a match, the component might not be set up as standaloneLength of output: 688
Script:
#!/bin/bash # Description: Verify SearchFilterComponent is set up as a standalone component # Test: Check if SearchFilterComponent's @Component decorator includes standalone: true ast-grep --lang typescript --pattern '@Component({ $$$ standalone: true, $$$ })' app/shared/search-filter/search-filter.component.tsLength of output: 233
Script:
#!/bin/bash # Description: Locate SearchFilterComponent and verify it is set up as a standalone component # Step 1: Find all files named search-filter.component.ts search_files=$(fd --extension ts --basename 'search-filter.component.ts') if [ -z "$search_files" ]; then echo "No files named search-filter.component.ts found in the repository." exit 1 fi # Step 2: Check each found file for standalone: true in the @Component decorator for file in $search_files; do echo "Checking $file for standalone: true..." result=$(ast-grep --lang typescript --pattern '@Component({ $$$ standalone: true, $$$ })' "$file") if [ -n "$result" ]; then echo "✅ $file is a standalone component." else echo "❌ $file is NOT set up as a standalone component." fi doneLength of output: 505
Script:
#!/bin/bash # Description: Locate SearchFilterComponent and verify it is set up as a standalone component # Step 1: Find all files named search-filter.component.ts search_files=$(fd --extension ts --base-name 'search-filter.component.ts') if [ -z "$search_files" ]; then echo "No files named search-filter.component.ts found in the repository." exit 1 fi # Step 2: Check each found file for standalone: true in the @Component decorator for file in $search_files; do echo "Checking $file for standalone: true..." result=$(ast-grep --lang typescript --pattern '@Component({ $$$ standalone: true, $$$ })' "$file") if [ -n "$result" ]; then echo "✅ $file is a standalone component." else echo "❌ $file is NOT set up as a standalone component." fi doneLength of output: 477
Script:
#!/bin/bash # Description: Locate SearchFilterComponent and verify it is set up as a standalone component # Step 1: Find all files named search-filter.component.ts using rg search_files=$(rg --files --glob 'search-filter.component.ts') if [ -z "$search_files" ]; then echo "No files named search-filter.component.ts found in the repository." exit 1 fi # Step 2: Check each found file for standalone: true in the @Component decorator for file in $search_files; do echo "Checking $file for standalone: true..." result=$(ast-grep --lang typescript --pattern '@Component({ $$$ standalone: true, $$$ })' "$file") if [ -n "$result" ]; then echo "✅ $file is a standalone component." else echo "❌ $file is NOT set up as a standalone component." fi doneLength of output: 2275
src/main/webapp/app/overview/course-faq/course-faq.component.ts (7)
3-4: LGTM!The added imports for
debounceTime,map,BehaviorSubject,Subject, andSubscriptionare appropriate and correctly utilized in the component.
18-18: LGTM!The import of
SearchFilterComponentis correctly added to support the new search functionality.
26-26: LGTM!Including
SearchFilterComponentin theimportsarray ensures it is available within the component. This is correctly implemented.
42-42: LGTM!Initializing
searchInputas aBehaviorSubjectto manage search queries is appropriate and follows best practices.
93-93: LGTM!Using
defineSearchedAndFilteredFaqwith the current search value ensures that both filters and search queries are applied consistently when toggling filters.
100-104: LGTM!The
applySearchmethod correctly filters the FAQs based on the search term, utilizing thehasSearchTokensmethod from theFaqService.
106-108: LGTM!The
setSearchValuemethod appropriately updates thesearchInputBehaviorSubject, effectively handling new search inputs.
src/main/webapp/app/overview/course-faq/course-faq.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-faq/course-faq.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-faq/course-faq.component.ts
Outdated
Show resolved
Hide resolved
# Conflicts: # src/test/javascript/spec/component/exam/participate/exam-navigation-sidebar.component.spec.ts
cremertim
dismissed stale reviews from SimonEntholzer, PaRangger, JohannesStoehr, and ole-ve
via
89236a5
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/test/javascript/spec/component/exam/participate/exam-navigation-sidebar.component.spec.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/exam/participate/exam-navigation-sidebar.component.spec.ts (1)
Pattern
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
🔇 Additional comments (3)
src/test/javascript/spec/component/exam/participate/exam-navigation-sidebar.component.spec.ts (3)
24-24: LGTM: Import statement for SearchFilterComponent.The import statement for
SearchFilterComponentis correctly placed and follows the project's import conventions. This import is necessary for mocking the component in the test module.
39-39: LGTM: Addition of MockComponent(SearchFilterComponent) to declarations.The
MockComponent(SearchFilterComponent)is correctly added to the declarations array, consistent with the mocking strategy used for other components. This allows theSearchFilterComponentto be used in the tests without its actual implementation, which is appropriate for unit testing theExamNavigationSidebarComponent.
Line range hint
24-39: Overall assessment: Changes look good, with a minor suggestion.The changes to this test file are minimal and well-implemented. The import and declaration of the mocked
SearchFilterComponentare correctly done and consistent with the existing code style. The existing tests remain unchanged, which is appropriate. Consider adding tests for any new interactions withSearchFilterComponentif applicable to ensure comprehensive test coverage.
src/test/javascript/spec/component/exam/participate/exam-navigation-sidebar.component.spec.ts
Show resolved
Hide resolved
Checklist
General
Client
Motivation and Context
I as a student want to be able to search for desired FAQ's based on the text of the FAQ
Description
Added a search bar to the Course Overview so a student can search via Text for the FAQ
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
Discussion:
Please give me feedback in the comments how you would expect the search functionalities to work:
a) Atleast 1 search token is contained within the FAQ
b) Every search token is contained within the FAQ
a) Search is performed on already filtered FAQ's (based on categories)
b) Search is performed on all FAQ's -> would remove filtering of the FAQ's
Summary by CodeRabbit
Summary by CodeRabbit
SearchFilterComponentfor handling search inputs.