-
Notifications
You must be signed in to change notification settings - Fork 839
feat: Contextually change stat color #7175
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Thanks! Feel free to disagree, would it be more intuitive if the API is just |
|
Thanks for suggesting that, I do think it’s less confusing to use |
|
@mscolnick, fyi on the API change. |
for more information, see https://pre-commit.ci
|
thank you! using var will raise pre-commit issues, we should use let / const instead. this snippet should help clean it up. const fillColors = {
increase: "var(--grass-8)",
decrease: "var(--red-8)",
};
const strokeColors = {
increase: "var(--grass-9)",
decrease: "var(--red-9)",
};
...
<TriangleIcon
className="w-4 h-4 mr-1 p-0.5"
fill={reverseColor ? fillColors.decrease : fillColors.increase}
stroke={
reverseColor ? strokeColors.decrease : strokeColors.increase
}
/> |
for more information, see https://pre-commit.ci
akshayka
left a comment
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.
Thanks for the contribution — concept makes sense, but I have a comment on the API.
marimo/_plugins/stateless/stat.py
Outdated
| caption: Optional[str] = None, | ||
| direction: Optional[Literal["increase", "decrease"]] = None, | ||
| bordered: bool = False, | ||
| reverse_color: bool = False, |
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 API is too imperative. We should prefer something more semantic or declarative.
For example, goal: Literal["increase", "decrease"],
mo.stat(value=10, direction="increase", goal="increase")which leaves room for the "goal" to affect other behaviors in the future instead of just reversing a color. This is more legible than reverse_color=True (Reverse color of what? Why do I want to reverse colors).
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.
Thanks for this suggestion, after reading it I totally agree. I have updated the PR changing the parameter name to target_direction to make it clear that the target is associated with the direction. To keep the current behavior I set the default target to "increase". This has also simplified the implementation, so great comment.
for more information, see https://pre-commit.ci
akshayka
left a comment
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.
Thanks, this is better! One comment
marimo/_plugins/stateless/stat.py
Outdated
| direction: the direction of the statistic, | ||
| either `increase` or `decrease` | ||
| bordered: whether to display a border around the statistic | ||
| target_direction: The target direction of the statistic, | ||
| either `increase` or `decrease` |
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.
The comment for target_direction could benefit from being more verbose. Without more context it is difficult to understand the difference between direction and target_direction
Clarified the target_direction parameter description.
for more information, see https://pre-commit.ci
akshayka
left a comment
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.
Thanks for the changes, this looks great to me!
@dmadisetti, can you review this from an API owner's perspective?
📝 Summary
With this PR it possible to manually swap the color of the triangle in the
mo.statUI element. Previously the color was always green for increases and always red for decreases, but sometimes a decrease is "good" and one may want to display a green downwards facing triangle.🔍 Description of Changes
The
statfunction now has acolorargument that accepts the literals "positive" (green) or "negative" (red) to allow the user to set the color manually. If not specified the color will be as it is now, green for increase and red for decrease.📋 Checklist