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

Groupcard #2211

Merged
merged 3 commits into from
Aug 27, 2024
Merged

Groupcard #2211

merged 3 commits into from
Aug 27, 2024

Conversation

tuva-odegard
Copy link
Contributor

I samarbeid med Ragnhild har vi oppdatert GroupCard komponenten litt.
Har oppdatert padding. Vi tilbyr nå kun en størrelse/padding, og ikke noPadding. Det er mulig vi kan legge til padding = 'sm' | 'md' eller liknende i fremtiden, men det er for tidlig nå.

Har forbedret darkmode og lagt til GroupCardTitle og GroupCardFooter

@tuva-odegard tuva-odegard requested a review from a team as a code owner August 19, 2024 12:01
Copy link

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

@tuva-odegard tuva-odegard force-pushed the groupcard2 branch 3 times, most recently from fd2438a to a5d175f Compare August 19, 2024 12:21
Copy link

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

1 similar comment
Copy link

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

@HeleneKassandra
Copy link
Contributor

HeleneKassandra commented Aug 19, 2024

Ikke noe fan av å fjerne noPadding, hvertfall om 'md' er eneste valg - for det er alt for mye padding for vårt bruk.
Tillatt hvertfall ett utvalg av spacing variablene i såfall, ellers begrenses bruken veldig.

Sånn vil det se ut hos oss om denne endringen blir tvunget igjennom:
Skjermbilde 2024-08-19 kl  15 22 17

@tuva-odegard
Copy link
Contributor Author

@HeleneKassandra Flott med en mening! Vi har vært utrolig mye frem og tilbake, var design som endte med at vi kun skulle ha en variant. Syntes det så utrolig stort ut, er du sikker på at det er 24px? 😅 Er det fordi dere bruker en komponent inni som har sin egen padding/margin i tillegg?

Er så vanskelig, fordi vi ønsker å sette en padding som en standard sånn at de fleste ser like ut, og at vi har gjennomtenkt hva som ser bra ut. Vi ønsker ikke noPadding fordi det ser dumt ut i de fleste tilfeller, men blir jo noe annet hvis folk bruker andre komponenter inni som har egen padding.. Men da mister vi litt kontrollen på hvordan det "burde" se ut også?

@HeleneKassandra
Copy link
Contributor

HeleneKassandra commented Aug 19, 2024

@HeleneKassandra Flott med en mening! Vi har vært utrolig mye frem og tilbake, var design som endte med at vi kun skulle ha en variant. Syntes det så utrolig stort ut, er du sikker på at det er 24px? 😅 Er det fordi dere bruker en komponent inni som har sin egen padding/margin i tillegg?

Er så vanskelig, fordi vi ønsker å sette en padding som en standard sånn at de fleste ser like ut, og at vi har gjennomtenkt hva som ser bra ut. Vi ønsker ikke noPadding fordi det ser dumt ut i de fleste tilfeller, men blir jo noe annet hvis folk bruker andre komponenter inni som har egen padding.. Men da mister vi litt kontrollen på hvordan det "burde" se ut også?

Ja, det er 24px og med all padding/margin fjernet fra innholdet inni. Jeg skjønner hele diskusjonen dere sitter i, men det må være en mellomting mellom å fjerne noPadding så vi kan styre paddingen på innholdet selv, og bare gi oss ett valg.
Kan man ikke tillate at man bruker spacing-variablene i ffe?

24px har forøvrig aldri vært en naturlig default på spacing så vidt jeg vet i FFE, vanligvis går man for 8 eller 16 når man skal sette en default. Spacing er også en av de tingene som oppleves annerledes i Figma enn i kode.

@tuva-odegard
Copy link
Contributor Author

Skjønner, har ikke vært borti det nok til å vite at 24px ikke er default, så ut til å være det på alle de andre kortene :-)

Dette er en åpen diskusjon, vi er åpne for inspill og denne PRen har ikke blitt godkjent enda engang. Det er fint å vite at du er for flere alertnativer, jeg kan skjønne det. Vi skal snakke mer med designeren vår i morgen og se om vi kan komme frem til noe. Vi har vært innom å ha en padding prop som det står i teksten over, mulig vi ikke skal release denne endringen uten.

Denne her endringen har propmptet en større diskusjon rundt at vi trenger bedre samarbeid med teamene, har vært utrolig mye "synsing" for å utrette groupCard.

Sånn her ser det ut i min browser med 24px padding med ikon, synes det ser merkelig ut om det ikke er noe mer på det skjermbildet du sendte 🤔

image

@tuva-odegard
Copy link
Contributor Author

@HeleneKassandra Hva er ønskescenarioet ditt? At vi har padding = 'sm' | 'md' | 'lg'? evt også 'none'?
Evt. fortsette med condensed prinsippet fra de andre kortene?
Eller foretrekker du noPadding, så alle teamene er helt fritt til å styre padding selv?
Eller noe helt annet? Greit å vite!

@HeleneKassandra
Copy link
Contributor

HeleneKassandra commented Aug 20, 2024

@HeleneKassandra Hva er ønskescenarioet ditt? At vi har padding = 'sm' | 'md' | 'lg'? evt også 'none'? Evt. fortsette med condensed prinsippet fra de andre kortene? Eller foretrekker du noPadding, så alle teamene er helt fritt til å styre padding selv? Eller noe helt annet? Greit å vite!

padding = 'xs' | ''sm' | 'md' | 'lg' | 'none' høres helt ideelt ut for meg! Tror jeg det er mer sannsynlig folk trenger xs enn lg, hvis man må kutte på noen størrelser.

Condensed kan nok også funke, som en middle ground hvis dere ikke vil åpne for padding som over ^ Evt som en første løsning frem til man har avdekket flere konkrete behov

@pethel
Copy link
Contributor

pethel commented Aug 20, 2024

@HeleneKassandra Hva er ønskescenarioet ditt? At vi har padding = 'sm' | 'md' | 'lg'? evt også 'none'? Evt. fortsette med condensed prinsippet fra de andre kortene? Eller foretrekker du noPadding, så alle teamene er helt fritt til å styre padding selv? Eller noe helt annet? Greit å vite!

padding = 'xs' | ''sm' | 'md' | 'lg' | 'none' høres helt ideelt ut for meg! Tror jeg det er mer sannsynlig folk trenger xs enn lg, hvis man må kutte på noen størrelser.

Condensed kan nok også funke, som en middle ground hvis dere ikke vil åpne for padding som over ^ Evt som en første løsning frem til man har avdekket flere konkrete behov

Issuen med condensed her er ju att også endre størelelse på icon. Annars hade man kunna droppa til førdel før padding. Føles som disse 2 konkurer med varndra. Annars att condensed kun endrer ikonet.

Copy link

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

@HeleneKassandra
Copy link
Contributor

@HeleneKassandra Hva er ønskescenarioet ditt? At vi har padding = 'sm' | 'md' | 'lg'? evt også 'none'? Evt. fortsette med condensed prinsippet fra de andre kortene? Eller foretrekker du noPadding, så alle teamene er helt fritt til å styre padding selv? Eller noe helt annet? Greit å vite!

padding = 'xs' | ''sm' | 'md' | 'lg' | 'none' høres helt ideelt ut for meg! Tror jeg det er mer sannsynlig folk trenger xs enn lg, hvis man må kutte på noen størrelser.
Condensed kan nok også funke, som en middle ground hvis dere ikke vil åpne for padding som over ^ Evt som en første løsning frem til man har avdekket flere konkrete behov

Issuen med condensed her er ju att også endre størelelse på icon. Annars hade man kunna droppa til førdel før padding. Føles som disse 2 konkurer med varndra. Annars att condensed kun endrer ikonet.

Ikonet er vel "custom" innhold som vi selv sender inn? Så da er det vel opp til hvert enkelt team å sørge for at ikonet har riktig størrelse? Eller har jeg misforstått?

Copy link

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

1 similar comment
Copy link

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

@@ -43,6 +46,15 @@ import { Heading2, Paragraph } from '@sb1/ffe-core-react';
</>
)}
</GroupCardElement>
<GroupCardFooter>
{({ CardAction }) => (
<>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ble ett fragment mer en vad som trengs her

@@ -5,7 +5,7 @@ import { BgColor, BgColorDarkmode } from '../types';

export interface GroupCardProps
extends Omit<React.ComponentPropsWithoutRef<'div'>, 'children'> {
shadow?: boolean;
noShadow?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Er du sikker på dette her? Mener det var slik før så endret @HeleneKassandra eller hsuker jag feil? Vi har skugga på de flesta av våra kort men vad emd andre team. Synes ikke vi skall jonglera den her fram og tilbake før mye.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅 Skjønner.. Var veldig mye lettere å få til mtp css og dark mode ved å gjøre det denne veien.. Tror det var derfor jeg byttet (pluss at vi helst vil at folk skal ha skygge, så tenkte det gir mening å legge til en noShadow kun hvis man vil..). Men helt enig at vi ikke burde bytte det rundt hvis det er allerede gjort! Ser nå at det er shadowpå de andre kortene.. Da må jeg jobbe litt til for å få til cssen

Copy link
Contributor

Choose a reason for hiding this comment

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

Vi varierer litt om vi har skygge eller ikke, mangler litt retningslinjer på det.

Men ja, tror kanskje jeg endret til shadow fra noShadow da jeg jobbet med basecard. Fint om det matcher andre kort komponenter :)

transition: all var(--ffe-transition-duration) var(--ffe-ease);
padding: var(--ffe-spacing-md);

&:last-child:not(:hover) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Vad var denne før? Når vi bruker hover vi bruka denne media query før slik att det kun skjer på dekstop med mus.

&:last-child {
&__title,
&__element {
&--no-separator {
Copy link
Contributor

Choose a reason for hiding this comment

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

den der kan slås ijop med element. Gjelder vell alle 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nei, tror ikke det? footer har ikke noe separator, er jo border-bottom

Copy link

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

Copy link

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

Copy link

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

@tuva-odegard tuva-odegard merged commit 3b4c8eb into develop Aug 27, 2024
3 checks passed
@tuva-odegard tuva-odegard deleted the groupcard2 branch August 27, 2024 08:54
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.

4 participants