Repository: Textualize/textual
Pull request: Add success/warning/error button variants
I kept the primary button as is on dark mode. I think I'd prefer if it was the blue/primary background colour on both light and dark mode, but not too fussed either way.

One kind of annoying thing is the "success" button is the only one where, on hover, the text colour changes.

https://user-images.githubusercontent.com/5740731/173611944-6ff07367-60a8-4c45-98ae-a83a2e8148fc.mov


Problem statement:
Button styles
The button widget could use a number of different semantic styles, namely default, success, warn, fail

These should be modelled in `Button.CSS` and use the design system colors. We also need dark variations for each.

In the css, the class `-success` etc should set the style. We could let the dev set the style with `Button(classes="success")` but consider some other parameter like `Button(variant="success")` or alt constructor like `Button.success()`

Hints from issue discussion:
What's the `-` prefix on `success` for? Seems surprising to me (and like something I'd forget about constantly).
It's a convention from a CSS style guide (I forget which) that says it's a modifier rule. Something you might toggle on and off.
I'm not really following where this would be used vs where it wouldn't. A "success" attribute on a button doesn't seem like something that's primarily intended to be toggled to me. I mean, it _can _ of course be added/removed, but so can any CSS class. I _do_ understand this convention it for things like focused vs not-focused, visible vs not-visible, active vs inactive (although don't particularly like it 😄).
When I said "toggle" that was probably misleading. It's more that it is a modifier, and more likely to be use dynamically.

Think of a `button` class that makes a div (or Widget) look like a button. That class is relatively static, you are unlikely to remove it once applied it and it is relatively self contained. Something like `-success` modifies `button`, and may be wrapped in some kind of condition. The `-success` class probably wouldn't do anything useful without the `button` class.

The prefix on `-success` suggests don't just add me to anything and expect it to indicate success.

It kind of makes sense to me. It was part of a larger CSS style guide, that I can't seem to locate now.

Commit message: Tidying Button widget-level CSS

Patch to review:
diff --git a/sandbox/buttons.css b/sandbox/buttons.css
index 77ff5c379d..cedf20ded1 100644
--- a/sandbox/buttons.css
+++ b/sandbox/buttons.css
@@ -1,12 +1,4 @@
-#foo {
-    text-style: underline;
-    background: rebeccapurple;
-}
-
-*:focus {
-    tint: yellow 50%;
-}
-
-#foo:hover {
-    background: greenyellow;
+Button {
+    padding-left: 1;
+    padding-right: 1;
 }
diff --git a/sandbox/buttons.py b/sandbox/buttons.py
index c90fe6b53d..daf1047a1a 100644
--- a/sandbox/buttons.py
+++ b/sandbox/buttons.py
@@ -1,21 +1,25 @@
+from textual import layout, events
 from textual.app import App, ComposeResult
-
 from textual.widgets import Button
-from textual import layout
 
 
 class ButtonsApp(App[str]):
     def compose(self) -> ComposeResult:
         yield layout.Vertical(
-            Button("foo", id="foo"),
-            Button("bar", id="bar"),
-            Button("baz", id="baz"),
+            Button("default", id="foo"),
+            Button.success("success", id="bar"),
+            Button.warning("warning", id="baz"),
+            Button.error("error", id="baz"),
         )
 
     def handle_pressed(self, event: Button.Pressed) -> None:
         self.app.bell()
-        self.log("pressed", event.button.id)
-        self.exit(event.button.id)
+
+    async def on_key(self, event: events.Key) -> None:
+        await self.dispatch_key(event)
+
+    def key_d(self):
+        self.dark = not self.dark
 
 
 app = ButtonsApp(
diff --git a/src/textual/message_pump.py b/src/textual/message_pump.py
index ca766e2efe..e733f77dd7 100644
--- a/src/textual/message_pump.py
+++ b/src/textual/message_pump.py
@@ -416,7 +416,6 @@ async def dispatch_key(self, event: events.Key) -> None:
         Args:
             event (events.Key): A key event.
         """
-
         key_method = getattr(self, f"key_{event.key}", None)
         if key_method is not None:
             if await invoke(key_method, event):
diff --git a/src/textual/widgets/_button.py b/src/textual/widgets/_button.py
index d93ebd4b6e..22e57f6691 100644
--- a/src/textual/widgets/_button.py
+++ b/src/textual/widgets/_button.py
@@ -1,57 +1,142 @@
 from __future__ import annotations
 
+import sys
 from typing import cast
 
+if sys.version_info >= (3, 8):
+    from typing import Literal
+else:
+    from typing_extensions import Literal  # pragma: no cover
+
 from rich.console import RenderableType
-from rich.style import Style
 from rich.text import Text, TextType
 
 from .. import events
+from ..css._error_tools import friendly_list
 from ..message import Message
 from ..reactive import Reactive
 from ..widget import Widget
 
+ButtonVariant = Literal["default", "success", "warning", "error"]
+_VALID_BUTTON_VARIANTS = {"default", "success", "warning", "error"}
+
+
+class InvalidButtonVariant(Exception):
+    pass
+
 
 class Button(Widget, can_focus=True):
     """A simple clickable button."""
 
     CSS = """
-
-
     Button {
         width: auto;
         height: 3;
+
         background: $primary;
         color: $text-primary;
-        content-align: center middle;
         border: tall $primary-lighten-3;
-        
+
+        content-align: center middle;
         margin: 1 0;
         align: center middle;
         text-style: bold;
     }
 
+    Button:hover {
+        background: $primary-darken-2;
+        color: $text-primary-darken-2;
+        border: tall $primary-lighten-1;
+    }
+
     .-dark-mode Button {
-        border: tall white $primary-lighten-2;
-        color: $primary-lighten-2;
         background: $background;
+        color: $primary-lighten-2;
+        border: tall white $primary-lighten-2;
     }
 
     .-dark-mode Button:hover {
         background: $surface;
     }
 
+    /* Success variant */
+    Button.-success {
+        background: $success;
+        color: $text-success;
+        border: tall $success-lighten-3;
+    }
 
-    Button:hover {
-        background:$primary-darken-2;
-        color: $text-primary-darken-2;
-        border: tall $primary-lighten-1;
+    Button.-success:hover {
+        background: $success-darken-1;
+        color: $text-success-darken-1;
+        border: tall $success-lighten-2;
+    }
+
+    .-dark-mode Button.-success {
+        background: $success;
+        color: $text-success;
+        border: tall $success-lighten-3;
+    }
+
+    .-dark-mode Button.-success:hover {
+        background: $success-darken-1;
+        color: $text-success-darken-1;
+        border: tall $success-lighten-3;
+    }
+
+    /* Warning variant */
+    Button.-warning {
+        background: $warning;
+        color: $text-warning;
+        border: tall $warning-lighten-3;
+    }
+
+    Button.-warning:hover {
+        background: $warning-darken-1;
+        color: $text-warning-darken-1;
+        border: tall $warning-lighten-3;
+    }
+
+    .-dark-mode Button.-warning {
+        background: $warning;
+        color: $text-warning;
+        border: tall $warning-lighten-3;
+    }
+
+    .-dark-mode Button.-warning:hover {
+        background: $warning-darken-1;
+        color: $text-warning-darken-1;
+        border: tall $warning-lighten-3;
+    }
+
+    /* Error variant */
+    Button.-error {
+        background: $error;
+        color: $text-error;
+        border: tall $error-lighten-3;
+    }
+
+    Button.-error:hover {
+        background: $error-darken-1;
+        color: $text-error-darken-1;
+        border: tall $error-lighten-3;
+    }
+
+    .-dark-mode Button.-error {
+        background: $error;
+        color: $text-error;
+        border: tall $error-lighten-3;
+    }
+
+    .-dark-mode Button.-error:hover {
+        background: $error-darken-1;
+        color: $text-error-darken-1;
+        border: tall $error-lighten-3;
     }
 
     App.-show-focus Button:focus {
         tint: $accent 20%;
     }
-
     """
 
     class Pressed(Message, bubble=True):
@@ -63,11 +148,22 @@ def __init__(
         self,
         label: TextType | None = None,
         disabled: bool = False,
+        variant: ButtonVariant = "default",
         *,
         name: str | None = None,
         id: str | None = None,
         classes: str | None = None,
     ):
+        """Create a Button widget.
+
+        Args:
+            label (str): The text that appears within the button.
+            disabled (bool): Whether the button is disabled or not.
+            variant (ButtonVariant): The variant of the button.
+            name: The name of the button.
+            id: The ID of the button in the DOM.
+            classes: The CSS classes of the button.
+        """
         super().__init__(name=name, id=id, classes=classes)
 
         if label is None:
@@ -79,6 +175,15 @@ def __init__(
         if disabled:
             self.add_class("-disabled")
 
+        if variant in _VALID_BUTTON_VARIANTS:
+            if variant != "default":
+                self.add_class(f"-{variant}")
+
+        else:
+            raise InvalidButtonVariant(
+                f"Valid button variants are {friendly_list(_VALID_BUTTON_VARIANTS)}"
+            )
+
     label: Reactive[RenderableType] = Reactive("")
 
     def validate_label(self, label: RenderableType) -> RenderableType:
@@ -100,3 +205,93 @@ async def on_click(self, event: events.Click) -> None:
     async def on_key(self, event: events.Key) -> None:
         if event.key == "enter" and not self.disabled:
             await self.emit(Button.Pressed(self))
+
+    @staticmethod
+    def success(
+        label: TextType | None = None,
+        disabled: bool = False,
+        *,
+        name: str | None = None,
+        id: str | None = None,
+        classes: str | None = None,
+    ) -> Button:
+        """Utility constructor for creating a success Button variant.
+
+        Args:
+            label (str): The text that appears within the button.
+            disabled (bool): Whether the button is disabled or not.
+            name: The name of the button.
+            id: The ID of the button in the DOM.
+            classes: The CSS classes of the button.
+
+        Returns:
+            Button: A Button widget of the 'success' variant.
+        """
+        return Button(
+            label=label,
+            disabled=disabled,
+            variant="success",
+            name=name,
+            id=id,
+            classes=classes,
+        )
+
+    @staticmethod
+    def warning(
+        label: TextType | None = None,
+        disabled: bool = False,
+        *,
+        name: str | None = None,
+        id: str | None = None,
+        classes: str | None = None,
+    ) -> Button:
+        """Utility constructor for creating a warning Button variant.
+
+        Args:
+            label (str): The text that appears within the button.
+            disabled (bool): Whether the button is disabled or not.
+            name: The name of the button.
+            id: The ID of the button in the DOM.
+            classes: The CSS classes of the button.
+
+        Returns:
+            Button: A Button widget of the 'warning' variant.
+        """
+        return Button(
+            label=label,
+            disabled=disabled,
+            variant="warning",
+            name=name,
+            id=id,
+            classes=classes,
+        )
+
+    @staticmethod
+    def error(
+        label: TextType | None = None,
+        disabled: bool = False,
+        *,
+        name: str | None = None,
+        id: str | None = None,
+        classes: str | None = None,
+    ) -> Button:
+        """Utility constructor for creating an error Button variant.
+
+        Args:
+            label (str): The text that appears within the button.
+            disabled (bool): Whether the button is disabled or not.
+            name: The name of the button.
+            id: The ID of the button in the DOM.
+            classes: The CSS classes of the button.
+
+        Returns:
+            Button: A Button widget of the 'error' variant.
+        """
+        return Button(
+            label=label,
+            disabled=disabled,
+            variant="error",
+            name=name,
+            id=id,
+            classes=classes,
+        )


Code review comments:
diff_hunk: @@ -100,3 +205,93 @@ async def on_click(self, event: events.Click) -> None:
     async def on_key(self, event: events.Key) -> None:
         if event.key == "enter" and not self.disabled:
             await self.emit(Button.Pressed(self))
+
+    @staticmethod
path: src/textual/widgets/_button.py
text: @user2:
shouldn't this be a `@user1`, so sub-classes of this `Button` class could re-use these helper constructors? :slightly_smiling_face: 

@author:
Changed