Conversation
There was a problem hiding this comment.
Pull request overview
Updates the promkit-widgets crate to v0.5.0 and aligns promkit to depend on it, while refining default/empty handling for text/listbox widgets and introducing a new feature-gated status widget.
Changes:
- Bump
promkit-widgetsto0.5.0and updatepromkitto depend on it (and bumppromkitto0.12.1). - Change
Text::from("")andListbox::default()to produce empty contents (and add unit tests for these behaviors). - Add a new
statusmodule/feature that wrapstext::Stateand applies severity-based styling.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| promkit/Cargo.toml | Bumps promkit version and pins promkit-widgets to =0.5.0. |
| promkit-widgets/src/text/text.rs | Adjusts empty-input parsing for Text and adds unit tests. |
| promkit-widgets/src/status.rs | Adds new status widget wrapper with severity-based styling. |
| promkit-widgets/src/listbox/listbox.rs | Makes Listbox::default() empty and adds a unit test. |
| promkit-widgets/src/lib.rs | Exposes status module behind the status feature. |
| promkit-widgets/Cargo.toml | Bumps crate version and adds the status feature (included in all). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async-trait = { workspace = true } | ||
| futures = { workspace = true } | ||
| promkit-widgets = { path = "../promkit-widgets", version = "=0.4.0" } | ||
| promkit-widgets = { path = "../promkit-widgets", version = "=0.5.0" } |
There was a problem hiding this comment.
promkit-widgets 0.5.0 introduces a new status feature/module, but promkit doesn't expose a corresponding feature (and promkit's all feature can't enable it). If promkit is intended to re-export all widgets behind its own feature flags, add a status = ["promkit-widgets/status"] feature here and include it in all so users/docs.rs can access promkit::widgets::status via promkit alone.
| impl State { | ||
| pub fn new<T: AsRef<str>>(text: T, severity: Severity) -> Self { | ||
| let mut state = Self { | ||
| text: TextState { | ||
| text: Text::from(text), |
There was a problem hiding this comment.
State keeps both severity and a derived text.config.style, but the style is only set once in new(). Since severity is pub, callers can mutate it later and the rendered style will silently be out of sync. Consider making fields private and adding a setter that updates text.config.style, or applying the severity style dynamically during rendering.
| impl Severity { | ||
| pub fn style(self) -> ContentStyle { | ||
| ContentStyle { | ||
| foreground_color: Some(match self { | ||
| Self::Success => Color::Green, | ||
| Self::Warning => Color::Yellow, | ||
| Self::Error => Color::Red, |
There was a problem hiding this comment.
The new status widget adds severity→style mapping, but there are no unit tests here to guard the mapping (e.g., that Warning sets foreground_color to yellow) and that State::new applies the style to the wrapped text::State. Adding a small test module in this file would help prevent regressions.
No description provided.