-
Notifications
You must be signed in to change notification settings - Fork 45
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 ModelProviderMenu component and reused it in PromptsendButton and MessageBase #730
Conversation
Can we close #729? |
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.
@hpatel292-seneca I've left some feedback. We need to replace this Menu with our custom implementation that supports SubMenus so you don't have to use an ad-hoc approach.
Thanks for moving the pull req over. I agree with Amnish re reusing code more. I suspect that might fix the only bug I found..there on first load if I click retry-with and search for "3b", the menu closes itself....on subsequent uses it seems ok |
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.
@hpatel292-seneca Just wrote some implementation ideas.
Deploying chatcraft-org with
|
Latest commit: |
5a286eb
|
Status: | ✅ Deploy successful! |
Preview URL: | https://eb183631.console-overthinker-dev.pages.dev |
Branch Preview URL: | https://issue-643-b.console-overthinker-dev.pages.dev |
Sweet this bug was fixed, functionality-wise current version is perfect from my perspective! |
Thanks @tarasglek, I found few bugs on mobile view while working it. I will open issues for those and if you agree to make those changes, I would to work on those. |
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.
@hpatel292-seneca This works well and code looks good to me. Thanks for following up!
@hpatel292-seneca can you "Resolve" the issues above that are now dealt with? It's hard to follow the current state of this PR otherwise. Let me know when this is ready to land. |
Hi @humphd, I can send a PR for the above issue if needed. However, if we merge the PR for the above issue first, this PR will likely encounter merge conflicts, as it also modifies the same file. Let me know how you'd like to proceed. |
I want to get this merged first then you can improve things in follow-ups. Please resolve whatever is done above, and file a new issue with the things to be fixed next, then resolve, so we can land this. |
@humphd I resolved all 'requested changes.' Sorry, I misinterpreted your message at first. |
@hpatel292-seneca this is a freaking huge quality of life improvement. Awesome work! |
Thanks @tarasglek |
This is Requested from #729