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

Add the Providers selection Menu Group to easy switch between providers #535

Merged
merged 12 commits into from
Mar 30, 2024

Conversation

Rachit1313
Copy link
Collaborator

@Rachit1313 Rachit1313 commented Mar 27, 2024

Closes #510

Tasks Completed:

  • Enable switching between providers from the ask menu for Desktop Prompt
  • Enable switching between providers from the ask menu for Mobile Prompt
  • Use MenuGroup from Chakra UI to group models and providers distinctly

Files Changed:
PromptForm.tsx

Screenshots:
image

@Rachit1313 Rachit1313 self-assigned this Mar 27, 2024
@humphd
Copy link
Collaborator

humphd commented Mar 28, 2024

We've got the same alignment issue here that you fixed elsewhere. All of the providers should be indented the same amount (left margin) and the icon only showing beside the selected provider. It shouldn't indent the text when the checkbox is beside it (e.g., the text should already be at that position).

Copy link
Collaborator

@kliu57 kliu57 left a comment

Choose a reason for hiding this comment

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

Some minor changes requested. Also you have to make the change from settings.provider using the apiUrl as the key to using name as the key once the hotfix #537 is merged, thanks.

src/components/PromptForm/PromptSendButton.tsx Outdated Show resolved Hide resolved
src/components/PromptForm/PromptSendButton.tsx Outdated Show resolved Hide resolved
@kliu57 kliu57 added this to the Release 1.7 milestone Mar 28, 2024
Copy link

cloudflare-workers-and-pages bot commented Mar 28, 2024

Deploying chatcraft-org with  Cloudflare Pages  Cloudflare Pages

Latest commit: d01fec4
Status: ✅  Deploy successful!
Preview URL: https://2027c207.console-overthinker-dev.pages.dev
Branch Preview URL: https://providers.console-overthinker-dev.pages.dev

View logs

@Rachit1313
Copy link
Collaborator Author

@kliu57 Do you want me change key based on name now or after the PR has been merged. I believe that it would be better to do it after merge as I would be able to test what I'm implementing. Or maybe.. I get this merged and get a PR ready to fix stuff which could be merged right after your PR.

@humphd, What would be the best approach for this?

Could you please also review the indentation changes based on your feedback?

@kliu57
Copy link
Collaborator

kliu57 commented Mar 28, 2024

@kliu57 Do you want me change key based on name now or after the PR has been merged. I believe that it would be better to do it after merge as I would be able to test what I'm implementing. Or maybe.. I get this merged and get a PR ready to fix stuff which could be merged right after your PR.

@humphd, What would be the best approach for this?

Could you please also review the indentation changes based on your feedback?

You cannot make the change until after my hotfix is merged.

@Rachit1313 Rachit1313 requested a review from kliu57 March 28, 2024 17:30
Copy link
Collaborator

@humphd humphd left a comment

Choose a reason for hiding this comment

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

I'm not loving the checkmark icon. I wonder if there is something better, maybe an arrow that points at the one

Maybe we should use a right-hand pointing chevron? https://react-icons.github.io/react-icons/search/#q=chevron

Something like:

Screenshot 2024-03-28 at 6 30 56 PM

Another thing I notice: when you change providers, it closes the menu, which is surprising, since you are probably going to want to pick a model from the new list. Can we make it so that choosing the provider doesn't close the menu?

Screenshot 2024-03-28 at 6 26 44 PM

@Rachit1313
Copy link
Collaborator Author

Rachit1313 commented Mar 29, 2024

@humphd , I changed the icon based on your feedback but not sure how can we achieve the second thing? I tried. using disclosure but even that didn't work. Maybe it has to close the menu after switching providers because it has to re render the model based on the change.

@Rachit1313 Rachit1313 requested a review from humphd March 29, 2024 03:53
@humphd
Copy link
Collaborator

humphd commented Mar 29, 2024

What if you call e.preventDefault() when it's a click on the provider?

@Rachit1313
Copy link
Collaborator Author

e.preventDefault()

Nope doesn't work

@Rachit1313
Copy link
Collaborator Author

@humphd , Found this which works for us.
chakra-ui/chakra-ui#3762

Copy link
Collaborator

@kliu57 kliu57 left a comment

Choose a reason for hiding this comment

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

Thanks for doing the refactoring I asked for, everything looks good to me.

@mingming-ma mingming-ma merged commit e8e55c5 into main Mar 30, 2024
4 checks passed
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.

Make it easier to switch between providers
4 participants