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

feat: ui.badge #973

Merged
merged 7 commits into from
Nov 8, 2024
Merged

feat: ui.badge #973

merged 7 commits into from
Nov 8, 2024

Conversation

AkshatJawne
Copy link
Contributor

@AkshatJawne AkshatJawne commented Oct 29, 2024

Closes #935 and Closes #868

(Also includes docs for ui.badge).

plugins/ui/docs/components/badge.md Show resolved Hide resolved
plugins/ui/src/js/src/widget/WidgetUtils.tsx Outdated Show resolved Hide resolved
Comment on lines 1 to 17
import React from 'react';
import {
Badge as DHCBadge,
BadgeProps as DHCBadgeProps,
} from '@deephaven/components';

export function Badge(props: DHCBadgeProps): JSX.Element {
const { variant } = props;
return (
// eslint-disable-next-line react/jsx-props-no-spreading
<DHCBadge {...props} variant={variant} />
);
}

Badge.displayName = 'Badge';

export default Badge;
Copy link
Member

Choose a reason for hiding this comment

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

Right now this wrapper isn't doing anything, and for some reason you're pulling variant out of the props just to pass it right into DHCBadge (which is unnecessary, since it was in props and you're passing all of them through already).
We do want to wrap text children though as per my other comments:

Suggested change
import React from 'react';
import {
Badge as DHCBadge,
BadgeProps as DHCBadgeProps,
} from '@deephaven/components';
export function Badge(props: DHCBadgeProps): JSX.Element {
const { variant } = props;
return (
// eslint-disable-next-line react/jsx-props-no-spreading
<DHCBadge {...props} variant={variant} />
);
}
Badge.displayName = 'Badge';
export default Badge;
import React from 'react';
import {
Badge as DHCBadge,
BadgeProps as DHCBadgeProps,
} from '@deephaven/components';
import { wrapTextChildren } from './utils';
export function Badge(props: DHCBadgeProps): JSX.Element {
const { children } = props;
return (
// eslint-disable-next-line react/jsx-props-no-spreading
<DHCBadge {...props}>{wrapTextChildren(children)}</DHCBadge>
);
}
Badge.displayName = 'Badge';
export default Badge;

Copy link
Contributor

Choose a reason for hiding this comment

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

This is on spectrum, but that sub-set of colors supported by variant is pretty weird. I wouldn't be opposed to expanding the implementation to manually pass in variant as backgroundColor UNSAFE_style then accept any color. We would also have to pick a proper contrasting fg color though.

Copy link
Contributor Author

@AkshatJawne AkshatJawne Nov 4, 2024

Choose a reason for hiding this comment

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

@dsmmcken Would this be something we wanted to look into more concretely?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure if we should hold this until that, or follow ticket. Because it would be a breaking change.

@mofojed thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Can be done in a follow up ticket. Doesn't have to be a breaking change, could type as variant: BadgeVariant | Color | None, and check if it's one of the built-in variants before falling back to apply as a colour (in JS).

mofojed
mofojed previously approved these changes Nov 5, 2024
margaretkennedy
margaretkennedy previously approved these changes Nov 6, 2024
dsmmcken
dsmmcken previously approved these changes Nov 7, 2024
@AkshatJawne AkshatJawne merged commit 55e8ce2 into deephaven:main Nov 8, 2024
17 checks passed
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.

ui.badge docs: ui.badge
4 participants