-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Feature: Predefined alerts with customizable colors and headings #1602
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
base: main
Are you sure you want to change the base?
Conversation
See docs/ui-components/alerts.md for how to use alerts.
|
Oh, this is super exciting work @pdmosses! I can aim to give a more in-depth review ... "as soon as possible" (maybe over the next few days? have some pending committee work 😅 ). As a big picture question: are you envisioning having this side-by-side with callouts, and having users decide which version they prefer? Or eventually pushing users to use alerts over callouts (or callouts over alerts)? I wonder if there's a way that we can reduce the maintenance burden of running two features like this in parallel (especially when there are accessibility considerations that limit our color palette choice). |
|
@mattxwang thanks for your initial response. I'm looking forward to your review!
I think it will be difficult to improve the accessibility of callouts – at least when they are configured using the colors defined by the theme. This is partly because all callouts use only the Alerts can (and currently do) use different shades for different colors, and don't have colored backgrounds. Moreover, the shades for the light and dark color schemes are independent. I hope their accessibility will be significantly better than callouts. And alerts are implemented entirely in SCSS, which might be easier to maintain (and understand!) than the Liquid+SCSS code needed to implement configuration-dependent callouts. I suggest to continue to support callouts, but without maintenance obligations. The docs should perhaps deprecate their configuration with some of the theme colors in the theme color schemes. |
|
Thank you for this PR, it really helped a lot! I only adjusted styling a bit to match old callouts since I liked them a bit more with a background |
|
@vladysor I'm glad you found the PR useful! Thanks for the feedback.
My main motivation for changing the styling from the old callouts (apart from simplifying the CSS) was to match GitHub Alerts. However, background colors can reduce contrast, which might be an accessibility concern for alert colors that are predefined by the theme. |
This PR adds alerts as an alternative to callouts. See also #1600 (comment).
The kinds and rendering of the predefined alerts are similar to the alerts supported on GitHub. However, the colors and headings can be customized, and further kinds of alerts can be added.
See the new docs page for Alerts in the preview for further details.