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

Allow two icons on iconcard #2505

Merged
merged 3 commits into from
Jan 29, 2025
Merged

Conversation

HeleneKassandra
Copy link
Contributor

Beskrivelse

Legger til muligheten til å ha to ikoner samtidig på iconCard.
image

Denne endringen lager en breaking change, da jeg fjerner iconPosition-prop som tidligere ble lagt til for at man kunne ha ikon på høyre-side og ikke bare venstre. Ved å tillatte 2 ikoner, så kan denne problemstillingen løses ved at man kun sender inn rightIcon-ikon

Motivasjon og kontekst

Vi har flere steder i Oversikt der vi bruker iconCard, men har 2 ikoner.
Dette er også allerede tillatt i StippledCard, så gjør ett forsøk på å legge det til i iconCard også.

Testing

Kjørt opp lokalt og sjekket i Storybook.

@HeleneKassandra HeleneKassandra requested a review from a team as a code owner January 27, 2025 15:26
@HeleneKassandra
Copy link
Contributor Author

For å få minst mulig endringer så beholdt jeg måten det ble rendret på i dag:

{iconElement}
{bodyElement}
{rightIconElement}

men her kunne man kanskje bare inlinet det?

@@ -13,16 +13,15 @@ export type IconCardProps<As extends ElementType = 'div'> = Omit<
'children'
> & {
/** Element of icon */
icon: ReactElement;
icon?: ReactElement;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hadde det gitt mening med leftIcon fremfor å beholde icon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ja, men jeg lot være for å unngå enda flere breaking changes. Per nå så er det egentlig kun de som bruker "iconPosition" som blir rammet av breaking change. Dette er vel også mønsteret i StippledCard per nå.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair, blir nok en gjennomgang av cards etterhvert så Figma og koden blir likere 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Er nok smart :) Ta gjerne kontakt om dere ønsker sparring på kort, vi har veldig mange forskjellige bruksområder og varianter hos oss i Team Oversikt, som jeg ville elsket å få inn i Designsystemet.

packages/ffe-cards-react/src/IconCard/IconCard.spec.tsx Outdated Show resolved Hide resolved
BREAKING CHANGE: removes "iconPosition"-related styling
BREAKING CHANGE: removes and replaces "IconPosition"-prop and styling with "rightIcon" prop.
@dagfrode dagfrode force-pushed the allow-two-icons-on-iconcard branch from 9251d97 to f30f94d Compare January 28, 2025 09:48
Copy link

Azure Static Web Apps: Your stage site is ready! Visit it here: https://black-beach-0d62d0d03-2505.westeurope.2.azurestaticapps.net

@HeleneKassandra HeleneKassandra merged commit 7c56452 into develop Jan 29, 2025
3 checks passed
@dagfrode dagfrode deleted the allow-two-icons-on-iconcard branch January 29, 2025 10:33
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.

3 participants