NEW Add transparent background state for Badge component #817
Conversation
|
Thanks for getting this PR so quick @robbieaverill. Mentioned in original issue... I presume that by default its square with slightly rounded corners, applying the class .badge-pill its rounded, and by applying .badge-transparent it removes the background (the three work against each other). |
There was a problem hiding this comment.
I'm happy with the code. I checked it out and I'm happy with playing with the knobs in the storybook.
I do wonder if this will be easy to use in practice. Some of the default text colours are light and some are dark. Perhaps this is just an issue with the classification of "inverted" but now you take out the background colour it gets more noticeable. This would cause an issue when showing a badge on a light background with the transparent option. You would have to manually add the inverted option depending on what style of badge it is.
I don't think this is a blocker - maybe a different issue at most. Merge if you agree.
Otherwise @clarkepaul I can give you a demo tomorrow - it's pretty easy to get going locally - @robbieaverill has already added the relevant changes to the pattern library in this PR.
|
Yeah, you're going to need to know how and when to use them, if it has a transparent background and white text then there is going to need to be a dark background within the UI. |
robbieaverill
force-pushed
the
pulls/1.4/transparent-inverted-badges
branch
from
fe639bb to
a5b33f1
Compare
|
Ok updated- now has |
|
Didn't check it out and play this time but the diff is mostly re-naming etc. |
There was a problem hiding this comment.
stateBadge sounds kind of weird as a prop name. It's not super clear what this does ... even after reading the doc. We should find a better name that is more descriptive about the purpose of the prop and what it does.
Is this inherently tied with inverted. Is there a scenario where one of those props would be true with the other one false?
| @@ -18,3 +20,4 @@ const props = { | |||
| * `message` (string): The string to display in the badge. | |||
| * `className` (string): Any extra classes to apply for the badge. | |||
| * `inverted` (boolean): If the colours should be inverted. | |||
| * `stateBadge` (boolean): For use in the context of versioning states (use with inverted). | |||
There was a problem hiding this comment.
It's nice to know in what context this is used, but I would rather know what it actually does.
There was a problem hiding this comment.
Yeah fair call. I'm not super happy about the way it's ending up either - would love a suggestion!
Basically inverted flips the background and text colours, and stateBadge or whatever you want to call it will remove the left and right padding from it, and make the background transparent, so it can be used for versioning state badges e.g. "LIVE", "DRAFT" etc. It's used in combination with inverted so you can get the right text colour, because the OOB badge component usually uses black text.
There was a problem hiding this comment.
What about unboxed or no-box?
There was a problem hiding this comment.
I try to avoid giving things names based on appearance as that might change over time, what happens if we designers add a background colour and padding back as we often want to evolve the design over time.
There was a problem hiding this comment.
I think we are better off keeping the name "state" as that shouldn't change over time were as its appearance will. I don't think we need to mention "Badge" again in "stateBadge" (maybe its required so it doesn't conflict with another class).
There was a problem hiding this comment.
state is very ambiguous in the context of a react component because you might be talking to the component's internal state or a redux state.
status or isStatusBadge might be better. Or maybe we create a <StateBade /> that wrap around the <Badge/> component.
There was a problem hiding this comment.
Perhaps that's the way to go here. <VersionedStateBadge /> could live in silverstripe/versioned-admin
There was a problem hiding this comment.
Is versioned-admin a dependency of campaign and CMS? If not you might be introducing some additional coupling.
There was a problem hiding this comment.
Fair point... which would auto enable history viewer. Maybe I’ll just put it in admin but as a new component
|
I'm going to move this to |
Allows React Badge component to be used for the designs in silverstripe/silverstripe-campaign-admin#77 for campaign admin