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

chatbot conversation nodes can contain a copy button #5125

Merged
merged 8 commits into from Aug 10, 2023

Conversation

fazpu
Copy link
Contributor

@fazpu fazpu commented Aug 8, 2023

Description

The PR introduces a new parameter show_chat_copy_button to the gr.Chatbot. When True, it exposes the Copy button for each converstation node.

image

@vercel
Copy link

vercel bot commented Aug 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
gradio ✅ Ready (Inspect) Visit Preview Aug 10, 2023 1:35pm

@pseudotensor
Copy link
Contributor

Cool!

@hannahblair
Copy link
Collaborator

This is really neat!

@freddyaboulton
Copy link
Collaborator

FYI @abidlabs - there was some hesitation against doing this in the past: #4911

@gradio-pr-bot
Copy link
Contributor

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
@gradio/chatbot minor
gradio minor
  • Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

chatbot conversation nodes can contain a copy button

Maintainers or the PR author can modify the PR title to modify this entry.

Something isn't right?

  • Maintainers can change the version label to modify the version bump.
  • If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

@gradio-pr-bot
Copy link
Contributor

The demo notebooks don't match the run.py files. Please run this command from the root of the repo and then commit the changes:

pip install nbformat && cd demo && python generate_notebooks.py

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Aug 8, 2023

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-5125-all-demos


You can install the changes in this PR by running:

pip install https://gradio-builds.s3.amazonaws.com/4eda1657fff8a049dd38c73efd1223a844f455e0/gradio-3.39.0-py3-none-any.whl

demo/chatbot_simple/run.py Outdated Show resolved Hide resolved
@abidlabs
Copy link
Member

abidlabs commented Aug 8, 2023

I tested the copy button and I think its fine to include. I still would disable it by default but providing developers with the option to include it if they want for their Chatbots seems like a good idea.

@hannahblair and I added some comments above. One more thing -- clicking on the copy button does not provide any sort of visual feedback that the button has been clicked. @fazpu could you do something similar to what we do for the JSON component where the copy button changes to a checkmark?

You can see that by running something like this:

with gr.Blocks() as demo:
    gr.JSON({"ab": "cd"})
    
demo.launch()

Once the changes have been made, I'm fine with merging, wdyt @freddyaboulton @hannahblair?

@freddyaboulton
Copy link
Collaborator

Sounds good to me @abidlabs @fazpu !

js/chatbot/static/Copy.svelte Outdated Show resolved Hide resolved
cursor: pointer;
padding: 5px;
top: 0;
right: 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.

added the positioning to improve readability and avoid unexpected behaviour

@@ -46,30 +45,25 @@
</script>

<button on:click={handle_copy} title="copy">
<span class="copy-text" class:copied><Copy /> </span>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this logic seemed to be unmaintained - it was not working. I simplified and removed all the outdated and non-necessary parts.

@fazpu
Copy link
Contributor Author

fazpu commented Aug 9, 2023

I tested the copy button and I think its fine to include. I still would disable it by default but providing developers with the option to include it if they want for their Chatbots seems like a good idea.

@hannahblair and I added some comments above. One more thing -- clicking on the copy button does not provide any sort of visual feedback that the button has been clicked. @fazpu could you do something similar to what we do for the JSON component where the copy button changes to a checkmark?

You can see that by running something like this:

with gr.Blocks() as demo:
    gr.JSON({"ab": "cd"})
    
demo.launch()

Once the changes have been made, I'm fine with merging, wdyt @freddyaboulton @hannahblair?

@abidlabs Thank you for the feedback! The logic was already there in the Copy component. It just needed a small refresh. Please have a look.

@abidlabs
Copy link
Member

abidlabs commented Aug 9, 2023

Tested, looks good. Thanks @fazpu for the great contribution 👍

Let's figure out the type error and then I believe we can merge in.

@abidlabs
Copy link
Member

Thanks @fazpu for this PR! Merging

@abidlabs abidlabs merged commit 80be7a1 into gradio-app:main Aug 10, 2023
12 checks passed
@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Aug 10, 2023

🎉 Chromatic build completed!

There are 0 visual changes to review.
There are 0 failed tests to fix.

@pngwn pngwn mentioned this pull request Aug 10, 2023
@luoweb
Copy link

luoweb commented Aug 14, 2023

I tested the copy button and I think its fine to include. I still would disable it by default but providing developers with the option to include it if they want for their Chatbots seems like a good idea.

@hannahblair and I added some comments above. One more thing -- clicking on the copy button does not provide any sort of visual feedback that the button has been clicked. @fazpu could you do something similar to what we do for the JSON component where the copy button changes to a checkmark?

You can see that by running something like this:

with gr.Blocks() as demo:
    gr.JSON({"ab": "cd"})
    
demo.launch()

Once the changes have been made, I'm fine with merging, wdyt @freddyaboulton @hannahblair?

no properties with gradio v 3.39.0, how to configure?

GradioUnusedKwargWarning: You have unused kwarg parameters in Chatbot, please remove them: {'show_copy_button': True}

@abidlabs
Copy link
Member

@luoweb you have to upgrade to the latest version of gradio:

pip install --upgrade gradio

@luoweb
Copy link

luoweb commented Aug 22, 2023

@luoweb you have to upgrade to the latest version of gradio:

pip install --upgrade gradio

it's OK now, Thanks

@huni0103
Copy link

huni0103 commented Sep 6, 2023

Hi there.

I would like to make an additional suggestion regarding Chatbot Copy.
In the Chatbot, I am replacing "<" , ">" with ">", "<", etc. to display "<" , ">".
However, when I copy the Chatbot text, the "" is also being copied.
I only want "<" and ">" to be copied as they appear in the Chatbot.

Furthermore, I'm representing newlines as "
", but when I copy the Chatbot text, it's copying them as "
" even though they are displayed as "\n" in the Chatbot.

Is there any way to address the issues mentioned above?"

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.

None yet

8 participants