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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Amend error toast and pill styling #4454

Merged
merged 13 commits into from Jun 15, 2023
Merged

Amend error toast and pill styling #4454

merged 13 commits into from Jun 15, 2023

Conversation

hannahblair
Copy link
Collaborator

@hannahblair hannahblair commented Jun 8, 2023

Description

This PR removes the gradient of the error toast and pill components. The background is now a solid red in light mode and the toast has a dark red border to the toast as seen in the screenshots.

Screenshot 2023-06-08 at 14 31 01 Screenshot 2023-06-08 at 14 30 48

Closes: #3418

Checklist:

  • I have performed a self-review of my own code
  • I have added a short summary of my change to the CHANGELOG.md
  • My code follows the style guidelines of this project
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

A note about the CHANGELOG

Hello 馃憢 and thank you for contributing to Gradio!

All pull requests must update the change log located in CHANGELOG.md, unless the pull request is labeled with the "no-changelog-update" label.

Please add a brief summary of the change to the Upcoming Release > Full Changelog section of the CHANGELOG.md file and include
a link to the PR (formatted in markdown) and a link to your github profile (if you like). For example, "* Added a cool new feature by [@myusername](link-to-your-github-profile) in [PR 11111](https://github.com/gradio-app/gradio/pull/11111)".

If you would like to elaborate on your change further, feel free to include a longer explanation in the other sections.
If you would like an image/gif/video showcasing your feature, it may be best to edit the CHANGELOG file using the
GitHub web UI since that lets you upload files directly via drag-and-drop.

@gradio-pr-bot
Copy link
Contributor

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

@pngwn
Copy link
Member

pngwn commented Jun 8, 2023

I like this better but i still think the pink background is weird (i'm no designer).

The only thing I am genuinely worried about with the background colour is red on pink, for accessibility reasons. I'm pretty sure the contrast wouldn't pass WCAG guidelines, although I haven't checked.

@abidlabs
Copy link
Member

abidlabs commented Jun 8, 2023

Really like the sharp border! Let's check the accessibility concerns @pngwn pointed out (perhaps we can make the red font darker if needed), but otherwise LGTM

@dawoodkhan82
Copy link
Collaborator

dawoodkhan82 commented Jun 8, 2023

Looks clean.

@dawoodkhan82
Copy link
Collaborator

Also I never really liked the big "X" button on the toast. I remember not even realizing it was a button at first, just thought it was an icon.

@@ -47,6 +47,8 @@ def __init__(
super().set(
# Colors
input_background_fill_dark="*neutral_800",
error_background_fill=colors.red.c200,
error_border_color=colors.red.c600,
Copy link
Collaborator

Choose a reason for hiding this comment

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

In order to affect the other themes, should we add these changes to base.py instead?

Copy link
Member

Choose a reason for hiding this comment

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

Good point, the changes are present in the default theme:

image

But not if you set theme="soft" or any of other built-in themes:

image

@pngwn
Copy link
Member

pngwn commented Jun 9, 2023

Regarding @dawoodkhan82's comment, what if the X icon was just an error icon and we had a standard close x button on the right (centred or top-right)? I think that would be clearer. It isn't obvious at first glance that the X icon is actually clickable. This would also work better if we added info warn variants in the future as we discussed recently.

Another alternative to the pinkish background is to make it more consistent with the dark mode one, a flat light background with red title text + icon. This would resolve the a11y concerns too. Just ideas though, we can also merge this in an iterate more in the future.

Some examples showing what I mean (although these don't match the style of gradio and some of the colours are a bit washed out):


I don't love the alignment on this but it is very readable. I also don't think this stands out enough but it is a decent start.

Screenshot 2023-06-09 at 13 28 21

I think these are actually quite good, although the line is kinda weird.

Screenshot 2023-06-09 at 13 28 41

This demonstrates how much cleaner the plain background is although the left border just looks out of place and these are slightly more compact than our error toast.

Screenshot 2023-06-09 at 13 29 50

These don't stand out nearly enough. The colours aren't saturates enough but the principle is nice.

Screenshot 2023-06-09 at 13 31 04

I quite like this one, especially if it stood out a little more (maybe a larger icon and something more dramatic than a sad face). That red bar at the bottom is actually a progress bare for when the toast will be removed, which is a nice touch imo. Especially if we added 'persistent' and 'non-persistent' toasts. A solid, unmoving bar (or the absence of a bar) could act a a subtle queue that the toast will remain. The presence of a moving bar suggests it will vanish.

Screenshot 2023-06-09 at 13 31 24

This is actually a really nice example of what I mean, although the error colour is not saturate enough.

Screenshot 2023-06-09 at 13 32 01

Just some ideas, like I say we could revisit this if we felt it was needed.

@hannahblair
Copy link
Collaborator Author

hannahblair commented Jun 12, 2023

Thanks for the comments all! I've made some changes here which I'd appreciate input on. Very much agree we should improve the design of this error so I'm glad you've put forward some thoughts. Here are a couple variants that I think look a lot better. Ive also checked that the colours used here for foreground and background are WCAG AA compliant.

Red background

Screenshot 2023-06-12 at 17 04 26

White background

Screenshot 2023-06-12 at 17 07 20

Dark mode (using default error background & white text)

Screenshot 2023-06-12 at 17 50 40

Notes:

  • I think font family consistency looks better (note that the mono font for the error description is gone)
  • Close icon on the right goes darker on hover
  • Currently it might look strange that there are two 'x' icons on the error toast; perhaps when we have the other variants in place (e.g. success, warning), this will seem more logical
  • All the colours above are WCAG AA+ compliant. The current errors we have in place aren't (see colour contrast reference here)
  • The colours we decide on in the toast would be updated in the status component as well

@hannahblair hannahblair assigned pngwn and hannahblair and unassigned pngwn Jun 13, 2023
@aliabid94
Copy link
Collaborator

Great work! It does throw me off a little is that there are two crosses.

Also I never really liked the big "X" button on the toast. I remember not even realizing it was a button at first, just thought it was an icon.

I had also felt this way, so I think it's definitely better to have an explicit close button, but maybe that can be the word 'close' like in pete's screenshots so there aren't two X's.

@pngwn
Copy link
Member

pngwn commented Jun 14, 2023

Love this! Could we figure out a way to make dark and light more more consistent? One is a lot more red than the other. I feel like the light mode on is more 'errory' than the dark mode one.

@dawoodkhan82
Copy link
Collaborator

Great work! It does throw me off a little is that there are two crosses.

I think changing the big "X" icon to an exclamation point icon could work

@hannahblair
Copy link
Collaborator Author

Thanks for the comments! I've also got @pngwn's changes in now and have made tweaks accordingly. I've got to this design so far.

Notes:

  • White background did look clean, but it just didn't fit with "conventional UX" of error toasts. If we're to add success/info and warning toasts, we'd want yellow/orange and green respectively, so it makes sense to keep the light red background. That said, I've done a WCAG contrast check and the light red and the slightly darker red text pass AAA requirements.
  • Big X icon has been changed to an exclamation point icon.
  • Removed drop shadow of toast. I think it looks better but if anyone is against this then lmk.

See below screenshots of current changes in the PR. Still open to any suggestions or changes :)

Light mode

Screenshot 2023-06-15 at 17 05 41 Screenshot 2023-06-15 at 17 05 21

Dark Mode
Screenshot 2023-06-15 at 17 05 47

Screenshot 2023-06-15 at 17 05 15

I'd like to extract the warning and info/success toast styles and icon into their own folders but for now, I'll keep them in this PR. That said, below are some vague designs I've made of how we'll adapt the future toasts.

Screenshot 2023-06-15 at 17 28 17

@abidlabs
Copy link
Member

UI looks great to me! However, I'm not being able to dismiss the toast when I click on the "x". Nothing happens, and nothing in the console either

@hannahblair
Copy link
Collaborator Author

@abidlabs accidentally removed that line! it's back in 馃憤

Copy link
Member

@abidlabs abidlabs left a comment

Choose a reason for hiding this comment

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

LGTM from my end, thanks @hannahblair!

@hannahblair hannahblair merged commit 7dc73d2 into main Jun 15, 2023
13 checks passed
@hannahblair hannahblair deleted the tweak-error-bg-styles branch June 15, 2023 17:51
@aliabid94
Copy link
Collaborator

Looks great!

I worry a little about how many theme variables will expand as a result of these, because we now have 10 theme variables for error, so we'll add 10 more for Warning and Info too? It's not a huge concern but we can't remove theme variables (for backwards compatibility) so it's something to keep in mind, cc @pngwn.

@pngwn
Copy link
Member

pngwn commented Jun 15, 2023

If they are necessary then there isn't much we can do if we want to keep things themeable, the shorthands and theme builder also simplify it somewhat but maybe we can figure out a way to reduce some of them or combine them in 4,0 (when we can remove stuff).

@hannahblair
Copy link
Collaborator Author

Definitely a good point @aliabid94. As @pngwn said, it may be an inevitable part of theming but it's worth considering how we can keep it as clean as possible going ahead, and remove what we can for 4.0 馃憤

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.

Design of the error labels have changed and they don't look good
6 participants