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

Re-use features from Ask menu in Retry sub-menu #643

Closed
humphd opened this issue May 31, 2024 · 8 comments
Closed

Re-use features from Ask menu in Retry sub-menu #643

humphd opened this issue May 31, 2024 · 8 comments
Assignees

Comments

@humphd
Copy link
Collaborator

humphd commented May 31, 2024

In #642 @menghif did some expert work to make the Ask menu more responsive and easier to use. We have a similar need in the Retry menu for AI Message responses. It would be interesting to see if we can re-use the same UX there, such that you can switch providers/models when retrying.

@hpatel292-seneca
Copy link
Collaborator

Hi @humphd, can I work on this?

@tarasglek
Copy link
Owner

Hi @humphd, can I work on this?
go ahead.

It would be nice to get matching search functionality too, otherwise retry-with openrouter is unusable

@humphd
Copy link
Collaborator Author

humphd commented Nov 7, 2024

@hpatel292-seneca go for it. If possible, try what @tarasglek suggests and port the whole search feature over too. Ideally we'd find a way to move that stuff into a component we share in both places.

@hpatel292-seneca
Copy link
Collaborator

Here's the corrected and improved version:


Hi @humphd, I tried to separate out the menu logic into a separate component, and it worked. Here is the demo:

2024-11-09.11-13-08.mp4

I just need to confirm one thing. I used a conditional in the component to render the MenuButton like this:

{menuButtonLabel &&
      React.isValidElement(menuButtonLabel) &&
      menuButtonLabel.type === SubMenu ? (
        <MenuButton
          as={Box}
          fontSize="1rem"
          fontWeight="normal"
          cursor="pointer"
          color="inherit"
          padding="0"
          background="none"
          border="none"
          aria-label={label || "Choose Model"}
          title={label || "Choose Model"}
          onClick={openOnHover ? undefined : onOpen} // Open on click if not openOnHover
          onMouseEnter={openOnHover ? onOpen : undefined} // Open on hover if openOnHover
          onMouseLeave={openOnHover ? onClose : undefined} // Close on hover out if openOnHover
        >
          {menuButtonLabel}
        </MenuButton>
      ) : (
        <MenuButton
          as={IconButton}
          size="sm"
          fontSize="1.25rem"
          aria-label={label || "Choose Model"}
          title={label || "Choose Model"}
          icon={<TbChevronUp />}
          onClick={openOnHover ? undefined : onOpen} // Open on click if not openOnHover
          onMouseEnter={openOnHover ? onOpen : undefined} // Open on hover if openOnHover
          onMouseLeave={openOnHover ? onClose : undefined} // Close on hover out if openOnHover
          borderLeftRadius="0"
        />
      )}

The reason for this approach is that both use cases require MenuButton to function differently. Initially, I tried to keep this logic separate and only shared the menuItem logic, but it didn’t work. This is because the two places where this component needs to render use different menu libraries: one uses @chakra-ui/react components, and the other uses @szhsin/react-menu. That’s why I had to make the entire Menu component reusable.

What do you think?

@tarasglek
Copy link
Owner

Freaking cool demo.

@humphd
Copy link
Collaborator Author

humphd commented Nov 9, 2024

This looks awesome @hpatel292-seneca. I wonder if, in a follow-up, we should convert all our menus to use this more flexible menu system we've built? That way it won't be such a hack to switch between.

@hpatel292-seneca
Copy link
Collaborator

Hi @humphd, I can work on a follow-up as you mentioned to convert all menus to use the custom menu component.

@hpatel292-seneca
Copy link
Collaborator

Hi @humphd Could you please close this issue? It is fixed in #730

@humphd humphd closed this as completed Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants