-
Notifications
You must be signed in to change notification settings - Fork 2
feat/required-settings #66
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
Conversation
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.
Pull Request Overview
This PR adds functionality to verify whether required solver settings have been filled, allowing the UI to prompt users to complete missing settings before solving a problem.
- Introduces an async function to check for unfilled required settings.
- Updates the SettingsView to highlight missing required settings and display a "saved" indicator.
- Updates the problem node callback to open the settings view if required settings are incomplete.
- Adjusts the fetchSolverSettings function signature for accurate typing.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/api/data-model/SolverSettings.ts | Added a function to check for missing required solver settings. |
src/components/solvers/settings/SettingsView.tsx | Updated UI elements to indicate missing settings and provide visual feedback. |
src/components/solvers/Graph/ProblemNode.tsx | Modified the solve callback to use the new required settings check. |
src/api/ToolboxAPI.ts | Corrected the return type of the fetchSolverSettings function. |
Comments suppressed due to low confidence (2)
src/api/data-model/SolverSettings.ts:47
- [nitpick] Consider renaming the variable 's_1' to a more descriptive name (e.g., 'setting') for improved code clarity.
.map((s_1) => s_1.name);
src/api/data-model/SolverSettings.ts:52
- [nitpick] Consider renaming the variable 's_2' to a more descriptive name (e.g., 'solverSetting') to enhance readability.
const filledSettings = problemDto.solverSettings.map((s_2) => s_2.name);
|
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.
Is it possible that we dont have "required" settings in the toolbox at the moment?
I tested the changes locally and could see the new feedback that is given when clicking the "save" button.
However, the highlighted settings and automated opening of the problem details did not appear. I tested k-means in VRP, and both PlanQK qubo solvers. I thought it might be because they are flagged as optional?
Yeah we don't have any right now. I have some flagged as such on my end but I have yet to push these changes. |
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.
intended behavior can now be observed thanks to
ProvideQ/toolbox-server#134
No description provided.