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

FE: Refactor chat control tooltips #797

Merged
merged 1 commit into from
Nov 9, 2024

Conversation

zeroliu
Copy link
Contributor

@zeroliu zeroliu commented Nov 9, 2024

  • Use radix tooltip to implement tooltips in the chat control. It's easier to maintain and handles more edge cases.
  • Made chat control icon bigger.
  • Made "unsaved history will be lost" message display conditionally based on auto save settings

2024-11-09 14 17 26
2024-11-09 14 17 07

@@ -247,40 +242,24 @@ const ChatInput: React.FC<ChatInputProps> = ({

<div className="chat-input-buttons">
{isGenerating && (
<Tooltip.Provider delayDuration={0}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed because there is no tooltip for these buttons

</button> */}
</>
<TooltipActionButton
onClick={onSaveAsNote}
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be onRefreshVaultContext?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

</>
<TooltipActionButton
onClick={onSaveAsNote}
Icon={<UseActiveNoteAsContextIcon className="icon-scaler" />}
Copy link
Owner

Choose a reason for hiding this comment

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

Since we are updating this, UseActiveNoteAsContextIcon is a legacy name, we can rename it to "RefreshIndexIcon" or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw we are using lucide icons. I updated them to use the library instead of custom Icons. WDYT?

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 can follow up to replace the rest of icons in the app.

Copy link
Owner

Choose a reason for hiding this comment

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

@zeroliu That would be great! These were added by a total frontend noob - me. Glad this will be cleaned up now!

Copy link
Owner

@logancyang logancyang left a comment

Choose a reason for hiding this comment

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

LGTM!

@logancyang logancyang merged commit 40cfc31 into logancyang:master Nov 9, 2024
2 checks passed
@zeroliu zeroliu deleted the zero/tooltip-control branch November 11, 2024 07:10
@logancyang logancyang mentioned this pull request Nov 13, 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
Development

Successfully merging this pull request may close these issues.

2 participants