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
Ensure Chatbot theme text size is set correctly #6958
Conversation
🪼 branch checks and previews
Install Gradio from this PR pip install https://gradio-builds.s3.amazonaws.com/ad9612510f3dd4af8e83868ba45d0e90570527a5/gradio-4.13.0-py3-none-any.whl Install Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@ad9612510f3dd4af8e83868ba45d0e90570527a5#subdirectory=client/python" |
🦄 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.
|
js/chatbot/shared/ChatBot.svelte
Outdated
@@ -306,7 +306,7 @@ | |||
background: var(--background-fill-secondary); | |||
width: calc(100% - var(--spacing-xxl)); | |||
color: var(--body-text-color); | |||
font-size: var(--text-lg); | |||
font-size: var(--body-text-size); |
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.
I think this will cause a breaking change for demos that use the default theme without specifying a text size, because the default body-text-size
is medium.
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.
Good point!
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.
Since the previous behaviour was broken, I'd be inclined to call this a bugfix.
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.
In theory yes, but it'd be a pretty jarring change for anyone upgrading their version of gradio. I'd suggest instead we keep the font-size of the chatbot text one size larger than the regular body-text-size. That way, people can still control the text size but without the breaking change. Is that possible using css vars?
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.
My last commit adds a function to tweak the body text sizes such that "sm", "md", "lg" theme settings equate to 14-16-20px body text sizes in the chatbot. For reference, the body text sizes would normally be 13-14-16px in other components. The chatbot component before this fix was behaving weirdly with 16-16-20px body text sizes.
My concern with this fix is that in a demo (with a theme) with a chatbot and multiple other components, the body text sizes won't be consistent throughout each component. Some custom CSS could be used to fix this but I'm wondering whether this might cause developer frustration as well.
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.
Nice! LGTM
My concern with this fix is that in a demo (with a theme) with a chatbot and multiple other components, the body text sizes won't be consistent throughout each component. Some custom CSS could be used to fix this but I'm wondering whether this might cause developer frustration as well.
That's true, although that is also the case right now with the default theme (chatbot size is larger than main body text) and we haven't heard any issues with it.
LGTM thanks @hannahblair! |
Description
We were hardcoding the font size to the large font variable in
Chatbot.svelte
, so thetext_size
was being ignored when setting the theme.Demo:
Closes: #6943
🎯 PRs Should Target Issues
Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.
Not adhering to this guideline will result in the PR being closed.
Tests
PRs will only be merged if tests pass on CI. To run the tests locally, please set up your Gradio environment locally and run the tests:
bash scripts/run_all_tests.sh
You may need to run the linters:
bash scripts/format_backend.sh
andbash scripts/format_frontend.sh