Skip to content

types wip #186

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

types wip #186

wants to merge 16 commits into from

Conversation

Shane98c
Copy link
Member

No description provided.

@Shane98c Shane98c marked this pull request as draft March 14, 2025 21:42
src/button.tsx Outdated
if (prefix) {
prefixHover = {
'&:hover > #prefix-span > #prefix': {
color: hoverColor,
...prefix.type.hover,
...(prefix.type as any).hover,
Copy link
Member

Choose a reason for hiding this comment

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

Ooh I didn't realize we did this -- so odd! So maybe a weird case where casting to any is okay?

Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this a little more, and it seems like a little util in this file could be helpful to accomplish the same thing while being a little more explicit?

const hasCustomHover = (comp: any): comp is { hover: ThemeUIStyleObject } =>
  !!comp?.hover

which you could then use for prefix and suffix like

        ...(hasCustomHover(prefix.type) ? prefix.type.hover : {}),

members: AvatarProps[]
direction?: 'horizontal' | 'vertical'
align?: Alignment | Alignment[]
spacing?: SizeKey | ResponsiveStyleValue<number | string>
Copy link
Member

Choose a reason for hiding this comment

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

Ooh ResponsiveStyleValue is so handy!

src/button.tsx Outdated
@@ -172,7 +196,7 @@ const Button = (
id='prefix-span'
sx={{ display: 'inline-block', ...prefixOffset }}
>
{prefix && prefix}
{clonedPrefix && clonedPrefix}
Copy link
Member

Choose a reason for hiding this comment

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

I know the change here is just a variable name update, but it seems like the && is not doing anything?

Suggested change
{clonedPrefix && clonedPrefix}
{clonedPrefix}

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

Successfully merging this pull request may close these issues.

2 participants