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

CopyButton prop for iconPosition #2173

Merged
merged 3 commits into from Aug 17, 2023
Merged

CopyButton prop for iconPosition #2173

merged 3 commits into from Aug 17, 2023

Conversation

KenAJoh
Copy link
Collaborator

@KenAJoh KenAJoh commented Aug 16, 2023

Description

Resolves #2157

Change summary

  • La til prop iconPosition for horisontal plassering av ikon. Samme API som Button

@changeset-bot
Copy link

changeset-bot bot commented Aug 16, 2023

🦋 Changeset detected

Latest commit: f6ee5fe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@navikt/ds-react Minor
@navikt/ds-css Minor
@navikt/aksel-stylelint Minor
@navikt/aksel Minor
@navikt/ds-tokens Minor
@navikt/ds-tailwind Minor
@navikt/aksel-icons Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Aug 16, 2023

Storybook demo

Endringer til review: 1

ccfabf28e | 48 komponenter | 310 stories

@KenAJoh KenAJoh merged commit 107d6f3 into main Aug 17, 2023
2 checks passed
@KenAJoh KenAJoh deleted the copybutton-placement branch August 17, 2023 11:21
@github-actions github-actions bot mentioned this pull request Aug 17, 2023
@@ -63,6 +63,11 @@ export interface CopyButtonProps
* @default 'Kopiert'
*/
activeTitle?: string;
/**
* Icon position in Button
Copy link
Contributor

Choose a reason for hiding this comment

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

Liten "b" kanskje? Siden det ikke er snakk om Button-komponenten.

Suggested change
* Icon position in Button
* Icon position in button

Comment on lines +128 to +145
const CopyIcon = () => {
return active ? (
<span className="navds-copybutton__icon">
{activeIcon ?? (
<CheckmarkIcon
aria-hidden={!!text}
title={text ? undefined : activeTitle}
/>
)}
</span>
) : (
<span className="navds-copybutton__icon">
{icon ?? (
<FilesIcon aria-hidden={!!text} title={text ? undefined : title} />
)}
</span>
);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Ingen big deal, men <span> er repetert, og det er muligens litt dårlig praksis å definere komponenter inni komponenter. Forslag:

Suggested change
const CopyIcon = () => {
return active ? (
<span className="navds-copybutton__icon">
{activeIcon ?? (
<CheckmarkIcon
aria-hidden={!!text}
title={text ? undefined : activeTitle}
/>
)}
</span>
) : (
<span className="navds-copybutton__icon">
{icon ?? (
<FilesIcon aria-hidden={!!text} title={text ? undefined : title} />
)}
</span>
);
};
const CopyIcon = (
<span className="navds-copybutton__icon">
{active
? activeIcon ?? (
<CheckmarkIcon
aria-hidden={!!text}
title={text ? undefined : activeTitle}
/>
)
: icon ?? (
<FilesIcon
aria-hidden={!!text}
title={text ? undefined : title}
/>
)}
</span>
);

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

Successfully merging this pull request may close these issues.

[Innspill til komponent]: <CopyButton />
2 participants