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

Add body color to gr.Accordion #8284

Merged
merged 6 commits into from
May 21, 2024
Merged

Add body color to gr.Accordion #8284

merged 6 commits into from
May 21, 2024

Conversation

hannahblair
Copy link
Collaborator

@hannahblair hannahblair commented May 14, 2024

Description

We've got a variable button_primary_text_color="white" in monochrome.py which causes the button text color in the Accordion to be white in monochrome themes. To prevent any breaking changes, I've set the text color in Accordion.svelte to body_text_color to fix this (as opposed to changing the value of that var). I can't see where this is an issue in other components at first glance.

Demo here

Closes: #8255

🎯 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

  1. 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

  2. You may need to run the linters: bash scripts/format_backend.sh and bash scripts/format_frontend.sh

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented May 14, 2024

🪼 branch checks and previews

Name Status URL
Spaces ready! Spaces preview
Website ready! Website preview
Storybook ready! Storybook preview
🦄 Changes detected! Details

Install Gradio from this PR

pip install https://gradio-builds.s3.amazonaws.com/9848219b26d2ea569451bf596992800c41d08d1d/gradio-4.31.4-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@9848219b26d2ea569451bf596992800c41d08d1d#subdirectory=client/python"

@hannahblair hannahblair changed the title Add body color to gr.Accordion Add body color to gr.Accordion May 14, 2024
@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented May 14, 2024

🦄 change detected

This Pull Request includes changes to the following packages.

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

With the following changelog entry.

Add body color to gr.Accordion

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.

@freddyaboulton
Copy link
Collaborator

This looks good but I think it'll probably break some user defined theme somewhere. Can we make this more tailored to the monochrome theme somehow? What came to my mind:

  1. Add a accordion_text_color and accordion_text_color_dark prop to the theme class
  2. By default, these prop values default to button_primary_text_color value
  3. For monochrome, accordion_text_color is body_text_color

What do you think?

@aliabid94
Copy link
Collaborator

Demo is private @hannahblair

Copy link
Collaborator

@aliabid94 aliabid94 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks @hannahblair!

@hannahblair
Copy link
Collaborator Author

This looks good but I think it'll probably break some user defined theme somewhere. Can we make this more tailored to the monochrome theme somehow? What came to my mind:

  1. Add a accordion_text_color and accordion_text_color_dark prop to the theme class
  2. By default, these prop values default to button_primary_text_color value
  3. For monochrome, accordion_text_color is body_text_color

What do you think?

thanks for the feedback! that's a good point. i've added a new accordion_text_color + -dark prop but we can't use the button_primary_text_color else we'll get this in light mode:

Screenshot 2024-05-21 at 13 17 50

I've used body_text_color as the default, but this still creates a breaking change I think.

Copy link
Collaborator

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

Thanks @hannahblair !

@hannahblair hannahblair merged commit 2d705bc into main May 21, 2024
8 checks passed
@hannahblair hannahblair deleted the monochrome-accordion branch May 21, 2024 22:52
@pngwn pngwn mentioned this pull request May 21, 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.

Accordion label is not displayed when theme is set to monochrome
5 participants