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

Added scenario to handle enable/disable of setup button. Also button text changed based on the scenario that either setup/update #156

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

msivasubramaniaan
Copy link

This PR helps to handle the below scenarios, and it would be nice to have as a user perspective.

  1. Button text changed between Setup/Update based on the installation status of the models
  2. If user selects the model which currently used then disabled the button

PFA

disable-setup-btn

…andled enable/disable of the button

Signed-off-by: msivasubramaniaan <[email protected]>
@deboer-tim
Copy link

Disabling the button when there's no change is nice, but if I don't realize that I've selected the current options it could be confusing why the button won't enable. IMHO either the button text needs to change as well, or replace the button with a label. Just something to indicate 'these are the current settings/configuration'.

@fbricon
Copy link
Collaborator

fbricon commented Nov 26, 2024

I'm dubious about this proposed change. The behavior is inconsistent between:

  • you select something that's already in config.json : button not disabled
  • you select something that's now in config.json : button disabled

I don't want to disable the button if the models are identical to what's in config.json, as I regularly rerun the setup because specific settings, other than the model id (context length, maxTokens ...) can change between extension builds, so I need to apply those changes.

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.

3 participants