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

tooltips on copilot #28

Closed
wants to merge 4 commits into from
Closed

tooltips on copilot #28

wants to merge 4 commits into from

Conversation

SanderGi
Copy link
Member

@SanderGi SanderGi commented May 28, 2024

Task: https://app.asana.com/0/1203067047205953/1206050253347928

Server PR: GooeyAI/gooey-server#348


@devxpy
Copy link
Member

devxpy commented May 28, 2024

Hmm, can't we just do the tooltip on the RenderedMarkdown widget, as opposed to defining the same thing on every input component?

In server code too then, we can pass the tooltip with st.write

DRY!

@SanderGi
Copy link
Member Author

SanderGi commented May 28, 2024

Hmm, can't we just do the tooltip on the RenderedMarkdown widget, as opposed to defining the same thing on every input component?

In server code too then, we can pass the tooltip with st.write

DRY!

We can, but it will require an extra layer in terms of a wrapper element to get the styling right. Shouldn't be a big performance hit since modern browsers are fast, but it is less readable. It is also a bit weird since we use the markdown component for the tooltip. Should I still move it to the markdown component?

We can't keep it in server code local to st.write because the inputs don't use st.write for their label.

@SanderGi
Copy link
Member Author

Position absolute does not work with display: contents, so I had to use the flexbox method

@devxpy devxpy force-pushed the main branch 2 times, most recently from 31aaf2d to f54027c Compare July 26, 2024 21:15
@anish-work
Copy link
Contributor

@devxpy PR is Updated with new changes

style={{ height: "16px", ...props.iconStyle }}
></i>
<span className="tooltip_text">
<RenderedMarkdown body={props.text} className="" />
Copy link
Member

Choose a reason for hiding this comment

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

This dependency loop is a bit confusing, can you refactor to have 3 widgets perhaps, RenderedMarkdownRaw (w/o tooltip), RenderedMarkdown and GooeyTooltip ?

@devxpy
Copy link
Member

devxpy commented Sep 19, 2024

we need a popover to implement the new workspace switcher and hence considering to ditch this and implement both popover and tooltip using https://github.com/floating-ui/floating-ui

@anish-work anish-work closed this Nov 20, 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.

3 participants