-
Notifications
You must be signed in to change notification settings - Fork 0
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
Alerts with gray text #33
Conversation
I prefer exactly the same as @kasperp 👍 |
Ditto for me 👍 (tho I also like hollow muted too) |
#2 - easiest to read of the first three Also like the last ones, and think they could make a nice change for the future - would like to see them in Ace to get a sense of how they could work. But, that's not for now :) Thanks Kasper :) |
Hmmm. There seems to be a disconnect between what Bootstrap says things should be and with what Ace is currently using. See the images below: My background is educational publishing, where accessibility was taken very seriously, especially since Pearson UK had to accommodate its content being used in the States were the laws are much tighter and awareness higher. Simply passing at AA for large text isn't good enough for the body copy. The Bootstrap Components page has different values - http://getbootstrap.com/components/#alerts. If we use these values then I'd have no qualms about keeping the Bootstrap colours and design. I can't see why we would ever want to 'mute' warning messages; they're warning, they should be front and centre and NOT blend in. If we fade them back too much they might end up as subtle as the breadcrumb trail. I definitely vote to go with the correct Bootstrap colours. |
@petefoll thanks for your feedback - it is great to have someone on the team that focus on and knows what it takes to be serious about accessibility. The bootstrap page you compared to is the latest release of Bootstrap which isn't the one Ace is currently using. Ace is using a Configit fork here http://configit.github.io/bootstrap/. However the commit for fixing the colors should be on our version here. 4386650 Would you mind running the tool again on the version of Bootstrap that is in ACE, and check if there is any differences? We could change colors of the alerts in our version to match the alerts in the latest version of Bootstrap. However I still think the version with the lighter gray text both looks better and is easier to read. I'm not convinced that changing the text color as well as the background color makes them easier to notice.
It is hard to disagree with this however there are other ways of make warning messages pop, than just changing the background color. You could animate them in, you could place them on top of another element, you could place them in a very prominent position on the screen (like top left), .... In Ace there are currently many places (e.g. compile page and update workitem page, etc.) where we do not use the alerts as alerts, but more as "informational boxes". In these cases you don't need the message to pop (as they will take focus from the real content), here the alternative alerts with the white background could be a better option. |
Okay. My screenshots were taken from current live Ace screens and gave: The latest Bootstrap, which does have a AA-passing, uses: We could do a quick fix to get the compliance by making the font in alerts a consistent grey like #534E53 (I can't get a clean sample of your lighter grey from the image). Which would give the below: |
d8114a2
to
1fd7f2b
Compare
@infoBorder: darken(spin(@infoBackground, -10), 7%); | ||
|
||
|
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.
This file contains the only real change, the rest are auto-generated dist and docs.
@petefoll the gray text color in my screenshot #2 is #555. @AButler I'll assign this to you. The change now has the gray text color. Once merged I'll update the docs on http://configit.github.io/bootstrap/ |
Thanks @AButler, docs are now updated. |
I know I'm a little bit late to the party here, but for what it's worth I'm not mad keen on the way #6 handles the join between the thick left border and the slim top and bottom borders. So when they're different colours, the border naturally ends up with a corner bevel which at this scale means an angled join rising 1 pixel along its 8 pixel length. |
We can easily encapsulate multiple html elements into one semantic component - infact I think we already do this to some extend. For example: <configit-alert type="error">You did something wrong</configit-alert> Could render into the browser as: <div class="alert-side alert-side-error">
<img src="error.gif">
</div>
<div class="alert-body alert-body-error">
You did something wrong
</div> Where the We use this technique extensively to reduce errors and make refactoring easier |
@configit/ux cc: @AButler
@petefoll noted that the alerts texts in Ace are hard (due to lack of contrast). This pull request explorers different options for solving this problem. Below is a list of screenshot with alternative solutions to the problem.
For reference this screenshot shows the current alerts.
Alternatives
Doesn't use background color for context: