Skip to content
This repository has been archived by the owner on Nov 4, 2019. It is now read-only.

✨ Add badge component #107

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

✨ Add badge component #107

wants to merge 1 commit into from

Conversation

XuTheBunny
Copy link
Contributor

Motivation

Adds badge component for labels and stats.
image

Use Cases

  1. Kids First Data Tracker
    image

  2. Kids First Coordinator
    image
    image
    image

  3. Kids First Portal
    image

API changes

No API changes are made on this component.

Implementation Notes

Referencing Colors in the storybook and color options.

Rendering and Storybook location

src/
└── components
    ├── Badge
    │   ├── __tests__/
    │   ├── Badge.jsx
    │   ├── Badge.story.jsx
    │   └── Badge.css
    └── index.js

Functional Tests

Test badge component under all the color options and on loading or not.

@XuTheBunny XuTheBunny added feature New functionality component Involves a component labels Apr 8, 2019
@XuTheBunny XuTheBunny self-assigned this Apr 8, 2019
@XuTheBunny XuTheBunny mentioned this pull request Apr 8, 2019
@dankolbman
Copy link
Contributor

dankolbman commented Apr 8, 2019

UIKit Storybook for Review

Built with commit 6a549bb

https://deploy-preview-107--kf-uikit.netlify.com

Copy link
Contributor

@bdolly bdolly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good start, a couple tweaks and I think we need to define the difference between a badge and a tag and what colors mean instead of maybe allowing users to use any color

src/components/Badge/Badge.jsx Outdated Show resolved Hide resolved
src/components/Badge/Badge.jsx Outdated Show resolved Hide resolved
src/components/Badge/Badge.jsx Outdated Show resolved Hide resolved
* Displays badge describing the file state
*/
const Badge = ({ className, state, color, loading }) => {
const badgeColor = color ? `bg-${color}` : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dankolbman should we leave this as a utility class or make specific BEM classes for color. I kind of think there should be specific BEM classes for colors that follow a standard like Badge--error Badge--success etc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that sounds good, if those different state -> colors are defined of course.
I think the idea here was to provide consistent state -> color mappings for content. Perhaps we want to allow the color to be over-ridable for use cases where app-specific states are needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so you're saying leave it as is for or change it up to make it BEM classes?

src/components/Badge/Badge.story.jsx Show resolved Hide resolved
src/components/Badge/__tests__/Badge-test.jsx Outdated Show resolved Hide resolved
stories/Colors/Colors.story.jsx Outdated Show resolved Hide resolved
stories/Colors/Colors.story.jsx Outdated Show resolved Hide resolved
@dankolbman
Copy link
Contributor

Story book is returning an error :(
Also, due to kids-first/kf-ui-data-tracker#155, maybe we want to rethink how these should be styled? @bdolly @XuTheBunny

Copy link
Contributor

@bdolly bdolly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small percy tweak again and waiting on some color discussion with @dankolbman

* Displays badge describing the file state
*/
const Badge = ({ className, state, color, loading }) => {
const badgeColor = color ? `bg-${color}` : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so you're saying leave it as is for or change it up to make it BEM classes?

src/components/Badge/Badge.story.jsx Show resolved Hide resolved
@XuTheBunny XuTheBunny force-pushed the add-badge branch 3 times, most recently from f5a9557 to 558e395 Compare April 18, 2019 13:59
@bdolly
Copy link
Contributor

bdolly commented Apr 19, 2019

maybe we want to rethink how these should be styled? @bdolly @XuTheBunny

Agreed, I think @allisonheath wanted to be part of that discussion as well as it involves brand standards

@allisonheath
Copy link
Member

I don't think I necessarily need to be there for the initial discussions/ideas, but if you all can come up with an option or two I can review/inform.

@XuTheBunny XuTheBunny force-pushed the add-badge branch 3 times, most recently from 73a0903 to 9c6e0b5 Compare April 22, 2019 19:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component Involves a component feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants