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

Initial commit for primitives package #2196

Open
wants to merge 83 commits into
base: master
Choose a base branch
from
Open

Initial commit for primitives package #2196

wants to merge 83 commits into from

Conversation

Jonas-C
Copy link
Contributor

@Jonas-C Jonas-C commented Apr 8, 2024

Fixes https://github.com/NDLANO/Issues/issues/3781
Fixes https://github.com/NDLANO/Issues/issues/3129

Dette er første del av overgangen til panda-css som styling-verktøy. Det tilbyr en hel haug med forskjellige måter å style ting på, men jeg foreslår at vi går en ganske restriktiv approach. I bunn og grunn tilbyr panda tre forskjellige måter å style på.

Styling-valg

Panda har støtte for shorthand-properties, d.v.s at bg tilsvarer background. Jeg tenker at det greieste hadde vært å holde seg unna dette, slik at man slipper å lære seg masse rare navn som ikke overlapper nevneverdig med vanlig css.

Vi har også muligheten til å være kjempestrenge, og kun tillate verdier som vi selv har definert. Dette vil gi oss et veldig uniformt designsystem, på bekostning av at vi ikke har like stor frihet. Jeg er litt usikker på dette, men inntil videre lener jeg mot nei.

Styled props / Styled system

Denne tilnærmingen ble populær samtidig som chakra-ui, og fungerer forsåvidt greit nok. Jeg frykter dog at overdrevent bruk av det vil føre til en helt ekstrem vendor-lockin.

return (
  <Wrapper backgroundColor="green" fontSize="16px" />
)

Template strings

Dette er det vi for det meste bruker i dag. Dette hadde vært det mest behagelige, og hadde krevd minst opplæring. Det har dog ikke like god token-støtte, og fører til at vi mister litt typesikkerhet.

const Wrapper = styled.div`
  background-color: green;
  font-size: 16px;
`

Object styles

Dette er en slags hybrid mellom template strings og styled props, og lar oss definere stiler på samme måte som style-propen i react gjør. Hvis vi går for denne har vi fortsatt et relativt sterkt skille mellom jsx og styling. Inntil videre virker det som at det er ganske grei interop mellom dette og emotion, slik at vi kan få til en gradvis overgang.

const Wrapper = styled('div', {
  base: {
    backgroundColor: "green",
    fontSize: "16px",
  }
};

return <Wrapper />
``

### Publisering av pakker for konsumenter som ikke bruker panda
Vi  publisere pakker  en måte som tillater at vi ikke krever at pakke-konsumenter ikke nødvendigvis  bruke panda. Panda har støtte for dette ved å spytte ut en css-fil som kan importeres, litt  samme måte som vi gjorde med scss. For en konsument som ikke ønsker å bruke panda vil man med andre ord slippa unna med det følgende i en css-fil:

```css
@layer reset, base, tokens, recipes, utilities;
// Denne vil inneholde alt av recipes, design-tokens og basestiler.
@import url('@ndla/primitives/dist/styles.css');
// Eventuelle pakker de også bruker. Denne css-filen bygger videre på `@ndla/primitives` sin css-fil.
@import url('@ndla/video-search/dist/styles.css');

Publisering av pakker for konsumenter som bruker panda

For brukere som ønsker å bruke panda vil vi legge ved såkalte buildinfo-filer, som en kan inkludere i panda.config.ts. Disse filene vil inneholde informasjon om alle filer som bruker panda i pakkene våre, slik at en ikke har behov for å scanne gjennom node_modules for å finne ut av det selv. Dette vil også sørge for at vi kun genererer css for komponenter vi faktisk bruker i frontendene våre.

@Jonas-C Jonas-C marked this pull request as draft April 8, 2024 10:09
@Jonas-C Jonas-C force-pushed the panda branch 2 times, most recently from 9b3e415 to 4c7aac9 Compare April 10, 2024 12:50
packages/primitives/src/Button.tsx Show resolved Hide resolved
packages/primitives/src/Icon.tsx Outdated Show resolved Hide resolved
packages/primitives/src/Input.tsx Outdated Show resolved Hide resolved
packages/primitives/src/preset/recipes/button.recipe.ts Outdated Show resolved Hide resolved
@Jonas-C Jonas-C force-pushed the panda branch 2 times, most recently from 29f35c7 to aa0b874 Compare April 12, 2024 11:25
@gunnarvelle
Copy link
Member

Synes dette ser bra ut. Object styling er veldig likt det vi gjør med emotion og siden vi kan innføre dette gradvis både i pakker og frontender er det vel berre å kjøre på. Dette vil kanskje i stor grad erstatte core?

@Jonas-C
Copy link
Contributor Author

Jonas-C commented Apr 12, 2024

Synes dette ser bra ut. Object styling er veldig likt det vi gjør med emotion og siden vi kan innføre dette gradvis både i pakker og frontender er det vel berre å kjøre på. Dette vil kanskje i stor grad erstatte core?

På sikt kan det erstatte core fullstendig. Tenker at en naturlig utvidelse av dette vil være å skrive om scss'en i core til global-css med panda.

Comment on lines 20 to 21
marginX: "0",
marginY: "0",
Copy link
Member

Choose a reason for hiding this comment

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

Fikser dette det eg fant med ikonet som var skeivt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tror det!

Copy link
Member

Choose a reason for hiding this comment

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

Ser ikkje sånn ut. Det er min-height som gjør det.

@katrinewi
Copy link
Contributor

katrinewi commented Apr 16, 2024

Jeg liker det! 😄 Nice at vi gradvis kan skrive oss over til det! Autocomplete fungerer også bra og måten å skrive object styles ligner jo mye på hva vi allerede har.

Noen ting:

  • Ser at strictTokens er et option man kan sette i config, vet du om det er mulig å styre for hvilke properties det skal gjelde for? For det kunne jo vært nice å kreve token-values for noen properties (font-styling blant annet, men at det ikke gjelder for width/height feks)
  • Burde shorthands settes til false i config? Hvis vi ønsker å unngå bruk av shorthand properties ..
  • Når jeg skriver inn en farge feks. som ikke finnes i preset så får jeg ingen feilmelding i editor, er dette mulig å få?
  • Hvordan skal vi gjøre conditional styling? Ser det er litt ulike approaches som er foreslått

@Jonas-C
Copy link
Contributor Author

Jonas-C commented Apr 16, 2024

Ser at strictTokens er et option man kan sette i config, vet du om det er mulig å styre for hvilke properties det skal gjelde for? For det kunne jo vært nice å kreve token-values for noen properties (font-styling blant annet, men at det ikke gjelder for width/height feks)

Nei, dessverre. Man må enten gå all-inn eller ingenting. Kan hende at vi kan få til noe greit med ESLint etter hvert.

Burde shorthands settes til false i config? Hvis vi ønsker å unngå bruk av shorthand properties ..

Litt usikker. Kan hende at vi faktisk bruker et par shorthands som er "greie", som f.eks paddingX og paddingY.

Når jeg skriver inn en farge feks. som ikke finnes i preset så får jeg ingen feilmelding i editor, er dette mulig å få?

Ja, man kan få det enten med ESLint eller strictTokens. Foreslår ESLint inntil videre.

Hvordan skal vi gjøre conditional styling? Ser det er litt ulike approaches som er foreslått

Tja, det kommer vel litt an på situasjonen egentlig. Generelt sett tipper jeg at vi kan slippe billig unna med data-attributter slik vi gjør de fleste steder nå.

import { RecipeVariantProps, cva, cx } from "@ndla/styled-system/css";
import { styled } from "@ndla/styled-system/jsx";

export const buttonRecipe = cva({
Copy link
Contributor

Choose a reason for hiding this comment

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

Sikkert en god grunn, men hvorfor har du gått for atomic isteden for config recipes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

config recipes er avhengig av å tracke komponentbruk, og det gjøres v.h.a navn eller regex. Med andre ord vil const StyledButton = styled(Button, {...}) ikke kunne trackes automatisk av panda. Tenkte det da var ryddigere å bare bruke cva.

@rauboti
Copy link
Contributor

rauboti commented Apr 19, 2024

Jeg syns jevnt over dette ser veldig bra ut. Som andre også alt har nevnt, har jeg kanskje mest trua på den objekt-stylingen, mye pga muligheten for å kunne gå gradvis over fremfor å ta alt på en gang.

Det eneste jeg kanskje kan tenke meg, som ikke alt er nevnt, er inndelingen av presets (både filer og definisjonen i index.ts-fila), som jeg mener er med på å heve terskelen for å sette seg inn i det slik det nå ligger. F.eks. hvis jeg finner en fil som heter spacing.ts, synes jeg det er naturlig at jeg finner all spacing der. Nå må jeg både leite i semanticTokens.ts og i spacing.ts for å skaffe oversikt (og bare et av de filnavnene sier tydelig at det er den som har med spacing å gjøre). Tilsvarende med farger. Måter å organisere det på som jeg personlig mener det er lettere å følge og se logikken i:

  • Slavisk legg alt som har med farger i farge-fil, alt som har med spacing i spacing-fil, osv
  • I tilfeller hvor det gir liten mening å samle alt (det er jo en logikk i oppdelingen du har gjort), lag en base-fil for basevalues (eventuelt en mappe med base value filer, dersom det blir mye), samle base verdiene der, også bruk spacing, colors etc som det "endelige resultatet". Preset kan da f.eks. se slik ut:
tokens: {
  colors: colors,
  spacing: spacing
  base: {
    colors: baseColors
  }
}

@Jonas-C
Copy link
Contributor Author

Jonas-C commented Apr 19, 2024

Jeg syns jevnt over dette ser veldig bra ut. Som andre også alt har nevnt, har jeg kanskje mest trua på den objekt-stylingen, mye pga muligheten for å kunne gå gradvis over fremfor å ta alt på en gang.

Det eneste jeg kanskje kan tenke meg, som ikke alt er nevnt, er inndelingen av presets (både filer og definisjonen i index.ts-fila), som jeg mener er med på å heve terskelen for å sette seg inn i det slik det nå ligger. F.eks. hvis jeg finner en fil som heter spacing.ts, synes jeg det er naturlig at jeg finner all spacing der. Nå må jeg både leite i semanticTokens.ts og i spacing.ts for å skaffe oversikt (og bare et av de filnavnene sier tydelig at det er den som har med spacing å gjøre). Tilsvarende med farger. Måter å organisere det på som jeg personlig mener det er lettere å følge og se logikken i:

* Slavisk legg alt som har med farger i farge-fil, alt som har med spacing i spacing-fil, osv

* I tilfeller hvor det gir liten mening å samle _alt_ (det er jo en logikk i oppdelingen du har gjort), lag en base-fil for basevalues (eventuelt en mappe med base value filer, dersom det blir mye), samle base verdiene der, også bruk spacing, colors etc som det "endelige resultatet". Preset kan da f.eks. se slik ut:
tokens: {
  colors: colors,
  spacing: spacing
  base: {
    colors: baseColors
  }
}

Dette er nok en blanding av et par forskjellige ting. For det første har vi nok godt av å legge inn en README som forklarer hvordan oppsettet fungerer. colors inneholder pdd de samme fargene som vi bruker i colors, men det skal den ikke alltid gjøre. Når det nye designet kommer vil den inneholde mye flere farger (red-100, red-200, red-300), og vil ikke lenger ha noen semantisk betydning. Ting som brand.primary vil da istedenfor legges i semanticTokens.

Jeg ser forsåvidt verdien i å definere alt det samme stedet, men det er ikke sånn panda fungererer. Man må enten splitte farger to steder (colors.ts, semanticTokens.ts) eller om splitte semantiske tokens over mange filer. Da synes jeg det første alternativet er enklere. Verdien du leter etter er enten i filen som korresponderer med token-typen, eller i semanticTokens.

@MaPoKen
Copy link
Contributor

MaPoKen commented Apr 19, 2024

Liker det!! Digger måten ting blir strukturert på, ser for meg at dette gjør livet enklere når den nye profilen også kommer. Bra jobba!

import { HelmetProvider } from "react-helmet-async";
import { I18nextProvider } from "react-i18next";
import { BrowserRouter } from "react-router-dom";
import { DocsPage, DocsContainer } from "@storybook/addon-docs";
import "./designmanual.scss";
import "./styles.css";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lokalt bruker jeg kun denne. Ting ser med andre ord ikke helt likt ut i preview/prod, da de gamle global-stylingene fortsatt ligger der. Synes det er fint å ha det slikt inntil videre, men på sikt må det endres.

Copy link
Contributor

Choose a reason for hiding this comment

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

Kunne det vært en idè å legge til en //ToDo her for å tydeliggjøre nettopp det? I tilfelle vi skulle få nytt team-medlem mellom når PR merges og når denne delen endres, tenker jeg på.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Denne endres nok om dager og ikke uker, altså. Har den bare lokalt hos meg så lenge det kun er jeg som skal jobbe med det.

Comment on lines +21 to +27
export const durations = defineTokens.durations({
superFast: { value: "100ms" },
fast: { value: "200ms" },
normal: { value: "400ms" },
slow: { value: "600ms" },
infinite: { value: "infinite" },
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Vet ikke hva folk tenker om navngivning her. Burde vi heller navngi animasjon basert på hvor det er "lurt" å bruke de?

Copy link
Contributor

Choose a reason for hiding this comment

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

Liker navngivninga slik. Eneste jeg synes er rar er superFast, men jeg trur ikke jeg har ideer om et bedre navn 🤷


import { defineGlobalStyles } from "@pandacss/dev";

export const globalCss = defineGlobalStyles({
Copy link
Contributor Author

@Jonas-C Jonas-C May 27, 2024

Choose a reason for hiding this comment

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

Har et lite håp om å droppe brorparten av den globale stylingen vi har i dag. Marginer og slikt burde egentlig bare gjelde når et element er plassert i en artikkel, ikke når vi har lyst til å bruke en liste på en hvilken som helst annen side. Det kommer nok garantert til å bli mer enn dette uansett

import { Meta, StoryObj } from "@storybook/react";
import { Icon } from "./Icon";

export default {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Denne skal flyttes, og er mao ikke så veldig viktig å se på (tror jeg)

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.

None yet

5 participants