BLW-34 Text-Alerts #4
Conversation
felix-berlin
changed the title
dist/css/blower.css
Outdated
| .alert--text-success { | ||
| --color-alert-text: #3F9A7A; | ||
| } | ||
| .alert--text-danger { | ||
| --color-alert-text: #D75052; | ||
| } | ||
| .alert--text-warning { | ||
| --color-alert-text: #DF8260; | ||
| } | ||
| .alert--text-info { | ||
| --color-alert-text: #00B8D4; | ||
| } |
There was a problem hiding this comment.
Bin ich kein Freund von. Das geht ein bisschen gegen BEM.
Mein Vorschlag wäre, dass man hier .alert--text und .alert--${status} kombiniert und dann die entsprechenden Custom Properties überschreibt.
Schau dir gerne mal an, wie das bei den Button-Klassen gelöst ist. Hier wird z.B. ein destruktiver Outline-Button wie folgt definiert:
<button class="button button--destructive button--outline">
Destructive
</button>Die Klassen sind also button, button--outline und button--destructive und nicht button, button--outline-destructive.
Ich würde an der Stelle mal zur Debatte stellen, ob wir nicht vom BEM-Standard hier etwas abweichen wollen und stattdessen zu is-text und is-success wechseln. Dann ist das Konsistent zwischen allen Komponenten. Könnte aber eben auch verwirrend sein, weil .button.is-text nicht dieselben Styles anwenden, wie .alert.is-text, sondern eben je nach Komponente unterschieden wird. An sich ein Teil von BEMIT.
Das wäre allerdings ein Breaking Change, der nicht in diesem Pull Request bearbeitet werden sollte, sondern in einem separaten Ticket.
There was a problem hiding this comment.
Ich glaube, ich verstehe, was du meinst, hatte es mit diesem Ansatz versucht, bin allerdings nicht zu dem gewünschten Ergebnis gekommen. Meine Erkenntnis war, es muss viel überschrieben werde und die Farben können leider nicht aus den vorhandenen vars recycelt werden (da schlicht nicht vorhanden). So, hab ich mich dann für diese Variante entschieden. Also in irgendeiner Weise müssen die Text-Varianten ihre eigene Farbe mitbringen, sonst ginge das wahrscheinlich ganz ok. Oder .alert--${status} speichert diese bereits, aber das ist auch etwas komisch, oder?
There was a problem hiding this comment.
Ok, ich hab mir mal Zeit genommen, um mir das anzuschauen und es entsprechend angepasst. Nun können wir problemlos mit class="alert alert--success alert--text" arbeiten.
…o blw-34-text-alerts # Conflicts: # dist/css/blower.min.css # package-lock.json
- Instead of having complete new classes, the `.alert--text` modifier now allows to make every alert to a text alert - Reduce specificity with `:where()` - Add dark and light to text alerts
|
@felix-berlin Bitte die Tests noch fixen. |
Dieser PR bringt eine neue Alert Variation
.alert--text-xyz.Unter http://blower-testground.test/components/alert.php gibt es Beispiele.
Die neue sowie ältere Varianten werden nun mittels Mixin und Loop generiert.