Skip to content
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

Fix: uncontrolled input in dashboard #895

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Animesh404
Copy link
Contributor

@Animesh404 Animesh404 commented Oct 26, 2024

What kind of change does this PR introduce?

a Bugfix for #889

Summary

  • Removed checked property: This avoids conflicting control by letting getInputProps fully manage the checkbox state.
  • Added defaultSettings which provides default values for all fields, preventing uncontrolled-to-controlled issues.

Checklist

  • My code follows the style guidelines of OpenAdapt
  • I have performed a self-review of my code
  • If applicable, I have added tests to prove my fix is functional/effective
  • I have linted my code locally prior to submission
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. README.md, requirements.txt)
  • New and existing unit tests pass locally with my changes

How can your code be run and tested?

poetry shell
python -m openadapt.entrypoint

OPENAI_API_KEY: '',
ANTHROPIC_API_KEY: '',
GOOGLE_API_KEY: '',
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the third place we are duplicating configuration setting names. Is there any way to populate this dynmamically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we populate this dynamically using config file but sometimes we might get undefined values which might be the reason for warning. So, I added these default values

@@ -32,19 +32,19 @@ export const Form = ({
<form onSubmit={form.onSubmit(saveSettings(form))}>
<Grid>
<Grid.Col span={6}>
<Checkbox label="Record window data" {...form.getInputProps('RECORD_WINDOW_DATA')} checked={form.values.RECORD_WINDOW_DATA} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this prevent the checkboxes from being checked appropriately depending on the configuration values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...form.getInputProps('RECORD_WINDOW_DATA') this alone should've marked this checked but yes now that I tested it again without checked= property it is not being marked appropriately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants