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

UNSAFE classname test #1826

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

Conversation

pavelklibani
Copy link
Contributor

@pavelklibani pavelklibani commented Dec 18, 2024

Description

  • All components are working with UNSAFE_className and UNSAFE_style (html, spirit components, other components)

Additional context

Issue reference

(UNSAFE_)className v elementType komponentach

@pavelklibani pavelklibani self-assigned this Dec 18, 2024
@github-actions github-actions bot added the refactoring A code change that neither fixes a bug nor adds a feature label Dec 18, 2024
Copy link

netlify bot commented Dec 18, 2024

Deploy Preview for spirit-design-system ready!

Name Link
🔨 Latest commit 3c5445a
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system/deploys/679246a7ac8faf00084b15a8
😎 Deploy Preview https://deploy-preview-1826--spirit-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 96 (no change from production)
Accessibility: 91 (no change from production)
Best Practices: 100 (no change from production)
SEO: 91 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 18, 2024

Deploy Preview for spirit-design-system-storybook ready!

Name Link
🔨 Latest commit 3c5445a
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-storybook/deploys/679246a769973800080f5111
😎 Deploy Preview https://deploy-preview-1826--spirit-design-system-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

props: HTMLAttributes<HTMLElement>;
};

export function useStyleProps<T extends StyleProps>(props: T): StylePropsResult {
const classNamePrefix = useContext(ClassNamePrefixContext);
const { UNSAFE_className, UNSAFE_style, ...otherProps } = props;
const { UNSAFE_className, UNSAFE_style, ElementTag, ...otherProps } = props;
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: I think we should pass here only the bool value and not the entire ElementTag/Component.

@pavelklibani pavelklibani force-pushed the refactor/ds-1176-unsafe-classname branch from 4b6dfe6 to ae48bf1 Compare January 16, 2025 15:19
@pavelklibani
Copy link
Contributor Author

squashed and rebased with main

@crishpeen
Copy link
Member

crishpeen commented Jan 16, 2025

I am still having a hard time doing what I want to do.

  1. I need output of DropdownTrigger (I added the needed stuff you introduce here to TooltipTrigger) <button class="button-unstyled" type="button">Trigger</button>
    a. <DropdownTrigger elementType="button" UNSAFE_className="button-unstyled" type="button"> I get this error
    Warning: React does not recognize the UNSAFE_className prop on a DOM element. and actually both UNSAFE_className and className are on the rendered element.
    b. <DropdownTrigger elementType="button" className="button-unstyled" type="button">, no className or UNSAFE_className applied at all
    c. const AvatarButtonAsDropdownTrigger = (props: SpiritNavigationActionProps) => ( <button {...props} className="button-unstyled" type="button" />); as local component and passed to DropdownTrigger as elementType works, but it is too much stuff to write.

  2. I need output of DropdownTrigger <button type="button" class="NavigationAction">trigger</button> (please note, that default elementType of NavigationAction is a)
    a. <DropdownTrigger elementType={NavigationAction} elementType="button"> does not work of course
    b. <DropdownTrigger elementType={(props) => <NavigationAction elementType="button" {...props} /> does not work
    c. const NavigationActionAsDropdownTrigger = (props: SpiritNavigationActionProps) => ( <NavigationAction {...props} elementType="button" />); works, but I am getting forwardref error
    d. code below works, but it is too much code again

const _NavigationActionAsDropdownTrigger = <E extends ElementType = 'a'>(
  props: SpiritNavigationActionProps<E>,
  ref: PolymorphicRef<E>,
): JSX.Element => {
  return <NavigationAction ref={ref} {...props} elementType="button" />;
};

const NavigationActionAsDropdownTrigger = forwardRef<HTMLButtonElement, SpiritNavigationActionProps<ElementType>>(
  _NavigationActionAsDropdownTrigger,
);

...
<DropdownTrigger elementType={NavigationActionAsDropdownTrigger as unknown as HTMLButtonElement}>

Simple example to try this is to rebase integration/header branch over this branch and try to simplify HeaderWithNavigation.tsx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring A code change that neither fixes a bug nor adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants