Skip to content
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

RFC: 0001 - Badge #9

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

RFC: 0001 - Badge #9

wants to merge 1 commit into from

Conversation

cellar-door
Copy link
Collaborator

Here is an initial draft of an RFC for the badge component. It can serve as an example for the RFC process, how it can be discussed, adopted, and then since it's relatively simple in design be implemented quickly.

Should this RFC process work well, we can fast follow with the top 40 components to build out the first set of RFCs for discussion.

This RFC was created by referencing the list of design systems who list a badge component on: https://open-ui.org/research/component-matrix/ and using their various implementations to try and draw commonality to be as universal as possible while taking into account i18n, a11y, and dx.

Copy link

@jamesnw jamesnw left a comment

Choose a reason for hiding this comment

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

This is a great and accessible RFC!

If we want to simplify an initial release, I would probably recommend dropping the ribbon and clickable. However, those are also reasonable features and help work through some of types of complexity.

|----------|------|---------|-------------|
| `variant` | `'primary' \| 'secondary' \| 'success' \| 'danger' \| 'warning' \| 'info'` | `'primary'` | Determines the visual style of the badge |
| `shape` | `'default' \| 'pill'` | `'default'` | Controls the border-radius of the badge |
| `position` | `null \| 'top-start' \| 'top-end' \| 'bottom-start' \| 'bottom-end'` | `null` | When used with a parent element, positions the badge accordingly |
Copy link

Choose a reason for hiding this comment

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

Would it make sense to adopt the anchor positioning syntax for position-area? That is likely overkill for this use case, as we just need to target the 4 corners, and we don't need any of the span or self keywords here.

I would recommend not mixing logical and physical keywords, and either picking one or supporting both.

|----------|------|---------|-------------|
| `variant` | `'primary' \| 'secondary' \| 'success' \| 'danger' \| 'warning' \| 'info'` | `'primary'` | Determines the visual style of the badge |
| `shape` | `'default' \| 'pill'` | `'default'` | Controls the border-radius of the badge |
| `position` | `null \| 'top-start' \| 'top-end' \| 'bottom-start' \| 'bottom-end'` | `null` | When used with a parent element, positions the badge accordingly |
Copy link

Choose a reason for hiding this comment

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

Do we also need an inset property that would control how much a dot badge would overlap with the parent container?

Edit- Oh, I just saw the --oui-badge-offset-x CSS variable, which I think exposes this. Not sure if an additional property is required.

| `position` | `null \| 'top-start' \| 'top-end' \| 'bottom-start' \| 'bottom-end'` | `null` | When used with a parent element, positions the badge accordingly |
| `visible` | `boolean` | `true` | Controls whether the badge is displayed |
| `aria-label` | `string` | `null` | Accessible label (required when badge only contains numbers or non-descriptive content) |
| `color` | `string` | `null` | Custom color for the badge (hex, rgb, or named color) |
Copy link

Choose a reason for hiding this comment

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

Could we handle this through CSS variables instead? Also, wouldn't we need both -text and -bg here?


## API

### Properties
Copy link

Choose a reason for hiding this comment

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

Should we also have a section to document any parts that would be exposed?

Choose a reason for hiding this comment

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

I think we should add parts and state sections, in this case there might not be a state (though maybe an :active ?)

Comment on lines +124 to +125
--oui-badge-padding-x: 0.65em;
--oui-badge-padding-y: 0.35em;
Copy link

Choose a reason for hiding this comment

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

Suggested change
--oui-badge-padding-x: 0.65em;
--oui-badge-padding-y: 0.35em;
--oui-badge-padding-inline: 0.65em;
--oui-badge-padding-block: 0.35em;

This isn't a blocker, but I think consistency with position would be useful.

| Slot | Description |
|------|-------------|
| `default` | The content of the badge (text or markup) |
| `count` | Alternative slot for badge content/count |
Copy link

Choose a reason for hiding this comment

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

Why does this exist instead of just using the default slot for the content?


### Composition

The Badge component will include a subcomponent called `oui-badge.ribbon` for the ribbon variant:
Copy link

Choose a reason for hiding this comment

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

Can you explain why this needs to be a separate element?


### Properties

| Property | Type | Default | Description |
Copy link

Choose a reason for hiding this comment

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

Most of these properties seem to control styling / appearance. In general, it's pretty limiting to set these directly on the element v. via css: (1) you need access to the element via HTML, (2) CSS is the standard tool for styling appearance, (3) CSS is highly configurable via the cascade and inheritance.

I think the desired functionality here could be implemented by using custom properties and style container queries.

| `visible` | `boolean` | `true` | Controls whether the badge is displayed |
| `aria-label` | `string` | `null` | Accessible label (required when badge only contains numbers or non-descriptive content) |
| `color` | `string` | `null` | Custom color for the badge (hex, rgb, or named color) |
| `count` | `number \| string` | `null` | Number or content to show in the badge |
Copy link

Choose a reason for hiding this comment

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

Why can this be specified both via a property and as child content? Seems like just as a child makes sense?

| `offset` | `[number, number]` | `[0, 0]` | Offset of the badge [x, y] in pixels |
| `size` | `'default' \| 'small'` | `'default'` | Size of the badge |
| `status` | `'success' \| 'processing' \| 'default' \| 'error' \| 'warning'` | `null` | Status indicator type |
| `show-zero` | `boolean` | `false` | Whether to show the badge when count is zero |
Copy link

Choose a reason for hiding this comment

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

Perhaps there should be a badge an a number-badge as there appear to be a set of features like this and overflow-count that make sense only when displaying a number?


| Event | Description |
|-------|-------------|
| `click` | Fired when the badge is clicked (if `clickable` is true) |
Copy link

Choose a reason for hiding this comment

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

This is a little confusing as any element can have a click handler. What does clickable false do? (pointer-events: none?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants