-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Handle the case where examples is null
for all components
#7192
Conversation
🪼 branch checks and previews
Install Gradio from this PR pip install https://gradio-builds.s3.amazonaws.com/f3e3c5c02f1069c1180004a46909309544f42523/gradio-4.16.0-py3-none-any.whl Install Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@f3e3c5c02f1069c1180004a46909309544f42523#subdirectory=client/python" |
null
null
for all components
🦄 change detectedThis Pull Request includes changes to the following packages.
With the following changelog entry.
Maintainers or the PR author can modify the PR title to modify this entry.
|
The static tests are failing even though those files are untouched by this PR. @whitphx do you have any idea of what could be going on? |
Hmm I can't find what's the problem |
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.
Looks good to me but noticed an issue with the empty video. Is it possible to test this via story book? Or is a unit test better here? CC @hannahblair
@@ -38,7 +38,7 @@ | |||
/> | |||
</div> | |||
{:else} | |||
<div>{value}</div> |
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.
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.
Makes sense
Thanks @whitphx. Should be fixed now, it was this issue (internal link) |
Thanks @freddyaboulton for the review, I'll fix those issues and add visual tests to storybook. |
Ok I've added stories for 4 components -- figured that this should be good to catch any regressions without bloating the number of visual tests. Should be good for another look @freddyaboulton @hannahblair |
js/checkbox/Example.svelte
Outdated
@@ -9,7 +9,7 @@ | |||
class:gallery={type === "gallery"} | |||
class:selected | |||
> | |||
{value.toLocaleString()} | |||
{value ? value.toLocaleString() : ""} |
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.
non-blocking + tiny nit: these expressions could be written as value && value.toLocaleString()
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.
Thanks for pointing this out @hannahblair. As I was testing this, I realized that my original approach won't work since value
for Checkbox
can be a boolean and if its False, that gets rendered as an empty string. Will revise
Nice! Stories look good to me - agreed that they'll be a good way of catching regressions 👌 |
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.
Very nice @abidlabs !
Thanks @freddyaboulton and @hannahblair for reviewing! |
I'm still having trouble displaying example images on mobile (iOS Chrome), 4.16.0 didn't work Also the solution here doesn't work. In requirements.txt I'm using: |
Hi @TonyAssi what issue are you running into? This PR didn't handle displaying images on mobile, it was related to setting examples to |
Example values can be
null
but this case wasn't really handled for any of the components. This PR handles the case for all components as part of fixing #5067. Also fixes the image distortion issue for examples mentioned here: #7180To test, at least for
gr.Textbox
andgr.Image
, use @radames's test code from #7180:Closes: #7180
Closes: #5067