[POC] Use `emotion` instead of `@emotion/core`

[sebald]

Since we now know that we "probably shouldn't use @emotion/core" (read the article), we should try to use emotion instead. @viktoria-schwarz always liked using the className better anyway than to have a dedicated css prop ... which btw also makes eslint/a11y work better.

RFC

What about the following?

import { styles } from '@marigold/system`;

const useStyles = styles('theme-section', {
  // base styles / normalization
});

const Component = props => {
  const classNames = useStyles(props.variant);
  return <div className={classNames}>{props.children}</div>;
}

PS: To uncomplicated the <Button> we could allow to pass an array of variants AND a single string?

Questions

1. Do we need a hook (for memoization + access theme)?
2. How does this affect SSR/SSG (add an example with this API to #360)?
3. List pros/cons of this approach regarding the css prop. (yes this is not a question, I know!)

API Inspiration

• Material UI Hook API
• glaze
• otion

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[viktoria-schwarz]

Ich komme nicht wirklich weiter was emotion angeht. Ich bin zwar dafür, dass wir das nutzen (classnames :tada:) aber mir fällt es schwer das in unsre bestehende Box in @marigold/system einzubauen. Sollen wir dafür nochmal telefonieren/uns zusammensetzen?

[sebald]

@viktoria-schwarz kannst du schon mal deine Idee hier posten? :)

[viktoria-schwarz]

Die Idee ist ja die CSS prop loszuwerden und mit einem className={} zu ersetzen - mir stellt sich schon dann die Frage, ob themeSection und variant ebenso komplett durch classNames ersetzt werden sollen, oder ob wir eine "Mischung" haben wollen.

Nach der emotion-Installation ist das ja recht leicht umzusetzen, einfach in die Box als className={cx(class1, class2)} (in der gewollten Reihenfolge...) die (im Theme?) definierten classNames einfügen. Soll es dann dort auch möglich sein, custom CSS reinzuschreiben? Also z.B. className={cx(class1, class2), 'border: 0'}?

Es wäre dann natürlich schön, wenn wir auf das "cx" verzichten können, das müsste man dann vermutlich mit einer Hook lösen. Ich denke wenn wir uns über die Fragen oben Gedanken gemacht haben, kriegen wir das schon eher hin :)

[viktoria-schwarz]

@sebald Wir haben nun einen Beispielbutton geschrieben (Button.tsx, um das mit den classNames zu testen. Wir sind uns aber nicht ganz einig über die API: wir erstellen die buttonStyles anhand von createStyles(), wo wir die ThemeSection (form) und custom styles eingeben. In den ButtonProps definieren wir die Variants (z.B. size), aber TypeScript meckert, wenn das ein string ist und kein Array. Die classNames übergeben wir dann dem HTML Button Element auch als String-Array. Im Theme steht dann

form: { // themeSection
    size: { // variant
        small: { // variant.x
            fontSize: 9,
        },
    },
},

aber trotzdem kommen nur die Base-Styles und die Custom styles an. Wo ist der Fehler? Fehlt nicht eigentlich noch die Ebene "button" (form.button.size.small)?

Außerdem sind wir uns nicht sicher wie man children übergibt. Steht jetzt etwas unschön bei den ButtonProps damit das klappt, aber da gibts sicher schönere Lösungen?

P.S. Die Tests wollen wir nächste Woche schreiben wenn wir das Gefühl haben das komplett verstanden zu haben :D

[sebald]

Wir sind uns aber nicht ganz einig über die API: wir erstellen die buttonStyles anhand von createStyles(), wo wir die ThemeSection (form) und custom styles eingeben

Etwas genauer? 😄

Das hier passt nicht ganz, die base styles beinhalten ja immer die Normalisierung.

https://github.com/marigold-ui/marigold/blob/ec11b2a7f0ae6e484c6da3d287dd5e3995b62c4a/packages/system/src/classnames-system.tsx#L18-L22

---

aber TypeScript meckert, wenn das ein string ist und kein Array.

Ihr habt die API ja so definiert?! 🤔

https://github.com/marigold-ui/marigold/blob/ec11b2a7f0ae6e484c6da3d287dd5e3995b62c4a/packages/system/src/classnames-system.tsx#L15-L17

---

Außerdem sind wir uns nicht sicher wie man children übergibt. Steht jetzt etwas unschön bei den ButtonProps damit das klappt, aber da gibts sicher schönere Lösungen?

Üblicherweise macht man das mit React.FunctionComponent bzw der Kurzform React.FC

export type ButtonProps = {
  size: string[]
}

export const Button:React.FC<ButtonProps> = ({ children, size }) => {}

BTW, wieso ist size ein Array?

---

P.S. Die Tests wollen wir nächste Woche schreiben wenn wir das Gefühl haben das komplett verstanden zu haben :D

Wenn ihr damit anfangt, findet ihr wahrscheinlich euer Problem bzgl des Type-Mismatch :P

[ti10le]

@viktoria-schwarz und ich haben nochmal besprochen wie wir uns die theme vorstellen. Wir dachten wir müssen jetzt erstmal wissen was wir überhaupt wollen. Alle styles sollen ja wie bisher in der theme stehen damit jede Component mit unterschiedlichen themes verwendet werden kann. Wir stellen uns die Struktur der theme ähnlich wie bisher vor. Ungefähr so:

const formStyles = createStyles('form', {
    button: {
      root: {
        position: 'relative',
        fontFamily: 'body',
        ..
      },
      size: {
        large: {
          lineHeight: '46px',
          paddingX: 5,
        },
        small: {
          lineHeight: '30px',
          paddingX: 3,
        },
      },
      type: {
        primary: {
          color: 'background',
          bg: 'primary',
        },
        secondary: {
          ...
        },
      }
    },
    input: {
      ...
    },
    label {
      ...
    },
);

const layoutStyles = createStyles('layout', {
  ...
});

Damit man am ende so etwas angeben kann: classNames={formStyles.button.size.small}

[ti10le]

So ungefähr stellen wir uns das in der Verwendung vor:

const classes = ["size.small", "variant.outlined"];
return (
    <Button classNames={classes}>test</Button>
)

[ti10le]

Und so in der component:

type ButtonProps = {
  size: string;
  variant: string;
};
export const Button: React.FC<ButtonProps> = ({
  size,
  variant,
  children,
  ...props
}) => {
  return (
    <button className={formStyles([size, variant])} {...props}>
      {children}
    </button>
  );
};

[sebald]

Ich kann nicht ganz folgen. Wollt ihr jetzt die Themen Struktur ändern? Die Angaben die ihr oben in createStyles gemacht haben, gehören dich in eine Theme, oder? Das sind ja weit mehr als base styles.

[ti10le]

Ja ich hatte auch vermutet das es ein bisschen unübersichtlich wird. Wir wollten es nur hier auch dokumentiert haben. Ja das aus meinem ersten von den drei Kommentaren mit dem createStyles soll zeigen wie wir uns ungefähr später die Theme vorstellen. Also das könnte dann z.B. ein Ausschnitt aus der b2b-theme sein.

[sebald]

Ich kann euch immer nur wieder daran erinner: Schreibt euch die Anforderungen auf und arbeitet euch daran voran. Das macht es für euch UND mich einfacher 😄

---

1. Habt ihr die Sachen in dem Feature-Branch? Ich verstehe die Namen nicht ganz: formStyles + layoutStyles.
2. Ich würde das nicht type nennen (form.button.type), weil es das HTML Attribut schon auf dem Button gibt (submit, ...). Warum nicht bei Variant bleiben so wie es auch in der Button Component ist?

[sebald]

https://github.com/marigold-ui/marigold/blob/ec11b2a7f0ae6e484c6da3d287dd5e3995b62c4a/packages/components/src/Button/Button.tsx#L30

gerade gesehen: Da es ein Hook is, muss das immer mit use beginnen! Man könnte die Funktion auch sonst createUseStyles nennen wenn das sonst zu Problemen führt.

[viktoria-schwarz]

Wir haben jetzt nochmal den aktuellen Stand gepusht auf den Branch test_emotion. Dadrin ist die Text-Component angepasst und vereinfacht und wir haben in der Testdatei einen Test, bei dem wir nicht verstehen warum er ständig fehlschlägt (das Problem hatten wir beim Button auch schon). Die base und custom styles funktionieren.

In der classnames-system.tsx sind wir uns nicht sicher, ob wir den Teil mit "basedOnVariants" richtig verstehen und anwenden.

Falls du wieder viele Rückfragen hast wäre es vielleicht leichter einfach eine halbe Stunde zu telefonieren wenn du dafür Zeit findest. Timo und ich sind zeitlich flexibel.

[sebald]

Ich schaue es mir an sobald ich Zeit hab, könnte aber ein bisschen dauern :)

Falls du wieder viele Rückfragen hast wäre es vielleicht leichter einfach eine halbe Stunde zu telefonieren wenn du dafür Zeit findest. Timo und ich sind zeitlich flexibel.

Das Problem ist eher, dass ich nicht weit voraussehen kann, wann ich Zeit habe :-/

[sebald]

See updates: https://github.com/marigold-ui/marigold/compare/40f0f4ba7a989c668cac3ebe58b50e181653b341...40d8763f39020d73d001bbe4c5fbd7c0a02784af

You need to use the "correct" context. We created a new context (this should eventually replace the current <MarigoldProvider>) that wasn't exposed.

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sebald]

Isn't that done? :)

[ti10le]

if #562 is done