-
Notifications
You must be signed in to change notification settings - Fork 3
feat: Implement basic article search feature #414
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
base: develop
Are you sure you want to change the base?
Conversation
Adds a new search bar to the top navigation, using a 'Data Socket' pattern with React Context and RTK Query. Includes internationalization support for new UI text and configures the proxy for the standalone backend API.
This commit establishes the foundational frontend architecture for the semantic search feature. It represents the second iteration of development, focusing on building out the UI components and data flow in preparation for backend integration. - Implements SemanticSearchContext for managing state and API interactions. - Integrates the main SemanticSearchContainer within the existing TopBarPopover. - Adds placeholder components, TypeScript types, and a Redux slice. - Includes internationalization keys for all new user-facing text.
This commit fixes a series of cascading test failures that occurred after introducing the semantic search feature. - Wraps test components in ArticleDataProvider to provide necessary context. - Removes a duplicate data-testid that caused an ambiguous element error.
4befb49 to
09c8a85
Compare
| throw new Error('useSemanticSearch must be used within a SemanticSearchProvider'); | ||
| } | ||
| return context; | ||
| } |
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.
this result handling should be done in the articleAPI which should then return SerarchResult objects.
This way the API response transformations are easier to maintain and the components in the App only use the defined types.
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.
Solved. II moved it to the API handling.
src/components/OnboardingComponents/SemanticSearch/SemanticSearchContainer.tsx
Outdated
Show resolved
Hide resolved
| # This is for development only. For production you need to replace this with the actual URL. | ||
| VITE_API_URL=http://localhost:8000 | ||
|
|
||
| # VITE_ARTICLE_API_URL= http://sc-010187l:8001/ | ||
| VITE_ARTICLE_API_URL=http://localhost:8001 |
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.
this is a bad comment (Ours above too 😅). Explain what the Variable is for.
Also don't expose network internal or development names to git 😬 bad enough that we do this for the IDP below.
| # This is for development only. For production you need to replace this with the actual URL. | |
| VITE_API_URL=http://localhost:8000 | |
| # VITE_ARTICLE_API_URL= http://sc-010187l:8001/ | |
| VITE_ARTICLE_API_URL=http://localhost:8001 | |
| # Development only. For production you need to replace this with the actual URLto the Backend API (https://github.com/SciCompMod/ESID-Backend). | |
| VITE_API_URL=http://localhost:8000 | |
| # Development only. For production you need to replace this with the actual URL to your Knowledgebase API (https://github.com/SciCompMod/ESID-Knowledgebase). | |
| VITE_ARTICLE_API_URL=http://localhost:8001 |
| # .env | ||
| .eslintcache | ||
| .env | ||
| .venv | ||
| __pycache__/ | ||
| /public/assets | ||
| /public/locales | ||
| /public/node_modules | ||
| /public/pdf.worker.min.mjs | ||
| .eslintcache | ||
| .env | ||
| .DS_Store |
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.
This looks like some leftovers from development?
I don't see why we would have python stuff in our repo.
not sure why the pdf worker is in here if it's still committed as the .js 🤔 is that on purpose?
| # .env | |
| .eslintcache | |
| .env | |
| .venv | |
| __pycache__/ | |
| /public/assets | |
| /public/locales | |
| /public/node_modules | |
| /public/pdf.worker.min.mjs | |
| .eslintcache | |
| .env | |
| .DS_Store | |
| .env | |
| .eslintcache |
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.
As for this change here, you are correct on your points. But I added the pdf worker here in an attempt to deal with the failing in loading PDFs, as there were two versions of the library. I still have not come to tangible fixes on this end.
8117a4b to
9b3f6b2
Compare
|
@NXXR feedback to Sarah when added (User Feedback 20, 2, 3) |
Adds a new search bar to the top navigation, using a 'Data Socket' pattern with React Context and RTK Query. Includes internationalization support for new UI text and configures the proxy for the standalone backend API.
Description
This Pull Request implements a new Article Search feature.
Related Issues
This feature was implemented based on the architecture requirements from the "Versatile Visual Analytics Framework for Exploration and Research" paper. No issue number was assigned.
Design Decisions
The implementation follows the existing architectural patterns of the application, specifically the "Data Socket" pattern.
ArticleDataContext.tsx: A new React Context was created to act as the "Data Socket". This component is responsible for all data fetching logic. It listens for search query changes and uses the RTK QueryuseLazyGetArticlesQueryhook to trigger API calls. This centralizes the data logic and keeps UI components "dumb".ArticleSearchBar.tsx: The search bar is a simple UI component. It gets the search results and the loading state from theArticleDataContextand sends user input back to the context. It does not fetch data itself.articleApi.ts: An RTK Query service was created to define the API endpoint (/api/articles) for fetching data. This keeps API definitions separate from UI and state logic.ArticleDataProvidercan be wrapped around the application inApp.tsxto provide the search functionality anywhere it's needed, and the backend is fully independent. For now, it is just a temporary element, as it will be modified after the development of the UI mockups.Performance & Quality
The search query is debounced within the component to prevent excessive API calls while the user is typing. No formal performance profiles have been attached for this initial implementation. It is an initial implementation with no true database. The only quality tests performed was checking the performance of the API calls when fetching the data to the search bar, which demonstrated to be happening efficiently.
Performance & Quality
Checklist
I, the author of this PR checked the following requirements for good software quality:
I, the reviewer checked the following things: