feat(Theme): handle contextual tokens by pylafleur · Pull Request #1066 · kronostechnologies/design-elements

pylafleur · 2025-01-13T19:26:01Z

Modification du système de compilation du thème pour gérer les tokens contextuels, qui permettent de spécifier une valeur alternative pour un token lorsqu'on se retrouve dans un contexte donné. Ces tokens sont de la forme token-name:context et sont permis dans les alias tokens uniquement.

Ex:

alias: {
    'text-heading-medium-font-size': 'font-size-500'
    'text-heading-medium-font-size:mobile': 'font-size-600',
}

Actuellement, les contextes pris en charge sont seulement les types d'appareils: mobile et tablet. Dans le futur, ce système pourra être bonifié avec certaines préférences utilisateur passées par le client: densité, contraste, animations, etc.

Pour que le système soit dynamique, le thème doit être recompilé chaque fois qu'il y a un changement dans le contexte. La fonction de compilation a été optimisée et garde en cache la dernière copie du thème compilé, ce qui permet d'éviter de multiples compilation si plusieurs tags <DesignSystem> sont utilisés côte-à-côte dans la même page.

Finalement, la documentation existante pour les thèmes dans le Storybook n'était pas compatible (chaque story est déjà wrapped dans un tag DesignSystem, c'est incohérent d'en ajouter un autre manuellement). Pour y remédier, j'ai ajouté un add-on qui permet d'appliquer le thème sur l'ensemble du Storybook, et qui fournit les bonnes valeurs au contexte racine.

pylafleur · 2025-01-13T19:27:55Z

 }

-const getDeviceContext = (deviceName: DeviceType | undefined = undefined): DeviceContextProps => {
+function getDeviceContext(deviceName: DeviceType): DeviceContextProps {


Les modifs dans ce fichier sont essentiellement pour limiter les changements de state si les valeurs n'ont pas changé, et ainsi limiter les recompilations du thème

github-actions · 2025-01-13T19:31:14Z

Storybook for this build: https://ds.equisoft.io/pr-1066/

github-actions · 2025-01-13T19:32:38Z

Webapp for this build: https://ds.equisoft.io/pr-1066/webapp/

savutsang · 2025-01-16T20:17:16Z

 export type TokenName = string;
 export type TokenValue = string;

+export type TokenContext = 'mobile' | 'tablet';


Je pense on devrait ajouter desktop aussi. Je pense pas que les theme sont necessairement desktop first. Et meme si on ne l'utilise pas vraiment, au moins la possibilite serait la pareil.

'my-token': 'red' 'my-token:desktop': 'blue' // vs 'my-token': 'blue' 'my-token:tablet': 'red' 'my-token:mobile': 'red'

pylafleur · 2025-01-22T16:49:18Z

En plus des petites corrections suggérées par Savut, j'ai retiré tous les changements qui touchaient aux add-ons du storybook. Comme le scope grandissait, on pourra le faire dans un PR à part. Comme les tokens contextuels ne sont pas encore utilisés dans les components, la problématique d'avoir plusieurs <DesignSystem> n'est pas visible.

meriouma

LGTM. Quelques petites questions, mais belle job!

meriouma · 2025-02-10T21:53:14Z

+// Context values should be added in order of importance. Later values will be applied over earlier values.
+function getTokenContext(
+    deviceContext: DeviceContextProps,
+    // @ts-ignore Future use


Tu pourrais toujours switcher à @ts-expect-error pour être certain de pas l'oublier, mais les chances sont faibles.

meriouma · 2025-02-10T22:13:01Z

-        component: {} as ResolvedTokenMap<ComponentToken>,
-    };
+    Object.entries(theme.alias).forEach(([contextualTokenName, tokenValue]) => {
+        const [tokenName, tokenContext] = contextualTokenName.split(':') as [AliasToken, TokenContext];


On peut avoir plusieurs contextes actifs, mais est-ce qu'on peut avoir plusieurs contextes dans un même token? Si oui je suis pas certain que c'est bien géré ici?

Mon idée c'était juste un contexte par token. Et aussi, le type ContextualToken utilise un template string qui valide que le nom du token contient un contexte valide, je ne crois pas que ça fonctionnerait si on permet un nombre arbitraire de contextes.

Ceci dit, si on a beaucoup de breakpoints, ça réduirait probablement le nombre de tokens. Je sais pas trop..!

meriouma · 2025-02-10T22:14:21Z

-        ...customizedTheme,
-        component: {} as ResolvedTokenMap<ComponentToken>,
-    };
+    Object.entries(theme.alias).forEach(([contextualTokenName, tokenValue]) => {


Peut-être?

Suggested change

Object.entries(theme.alias).forEach(([contextualTokenName, tokenValue]) => {

Object.entries(theme.alias).forEach(([maybeContextualTokenName, tokenValue]) => {

meriouma · 2025-02-11T04:00:52Z

-};
-
-export const equisoftTheme = buildTheme(equisoftThemeCustomization);
+export const equisoftThemeCustomization: ThemeCustomization = {};


Je me demande si ce layer sert encore à quelque chose vs faire themeCustomization = {} dans le ThemeWrapper, ou mettre le fallback à {} directement dans la signature de buildTheme? 🤔

Hmm, mais ça va peut-être fucker ceux qui importaient le equisoftTheme. Faudrait qu'ils passent explicitement undefined si on veut pas nécessairement exporter ça. Hmmmmmmm.

Mon idée c'était de rendre explicite le fait que le thème par défaut est bien le thème Equisoft, et non un thème "vide".

De plus, dans des discussions antérieures sur les tokens, on avait déterminé que le thème Equisoft divergeait un peu du mapping des autres thèmes et qu'il serait préférable qu'il ait des overrides par-dessus le mapping par défaut, ce qui éliminerait des overrides systématiques dans les autres thèmes. Ça ne s'est pas encore concrétisé mais je crois que c'est toujours d'actualité.

Tout ça pour dire que je le laisserais comme ça! 😛

meriouma · 2025-02-11T04:20:34Z

@@ -1,4 +1,4 @@
 export * from './equisoft';
-export * from './tokens';
+export * from './tokens/tokens';


Créer un index.ts?

Y'en a déjà un mais je pense que je ne voulais pas exporter toute la liste des tokens et les mappings par défaut. Mais j'imagine que ça serait pas ben grave de le faire non plus...

meriouma · 2025-02-11T04:37:26Z

 import { decorateWith, Decorator } from './decorator';

-export const DeviceContextDecorator: Decorator = decorateWith(DeviceContextProvider);
+export const DeviceContextDecorator: Decorator = decorateWith(DesignSystem);


Il sert à quoi en fait ce décorateur? Vu que le décorateur à la racine c'est le DS dans le preview 🤔

Il sert pu. Je crois qu'on avait mal compris comment les décorateurs s'appliquaient dans Storybook, ou encore que quelque chose a changé depuis de leur côté.

Dans tous les cas, je retire ça et je fais un cleanup des stories dans un jira à part. Pour l'instant, ça ne cause pas de problème de le laisser comme ça.

pylafleur added 3 commits January 13, 2025 13:39

feat(Theme): add snapshot for default theme

6434b30

feat(Theme): handle contextual tokens

b002d12

feat(Storybook): add toolbar menu for switching theme

4923f68

pylafleur requested a review from a team as a code owner January 13, 2025 19:26

pylafleur commented Jan 13, 2025

View reviewed changes

pylafleur temporarily deployed to github-pages January 13, 2025 19:30 — with GitHub Actions Inactive

pylafleur temporarily deployed to github-pages January 13, 2025 19:31 — with GitHub Actions Inactive

feat(Storybook): remove theme switcher, fix viewport breakpoints

ae54024

pylafleur temporarily deployed to github-pages January 13, 2025 21:44 — with GitHub Actions Inactive

pylafleur temporarily deployed to github-pages January 13, 2025 21:45 — with GitHub Actions Inactive

savutsang reviewed Jan 16, 2025

View reviewed changes

Comment thread packages/react/src/components/device-context-provider/device-context-provider.tsx Outdated

savutsang reviewed Jan 16, 2025

View reviewed changes

Comment thread packages/react/src/components/device-context-provider/device-context-provider.tsx Outdated

savutsang reviewed Jan 16, 2025

View reviewed changes

pylafleur added 4 commits January 21, 2025 15:22

Merge remote-tracking branch 'origin/master' into dev/DS-1277

ac69d7d

feat(Storybook): revert viewport addon

ff87bca

feat(Storybook): use refs for derived states

43638c2

feat(Storybook): add desktop to token contexts

b04f3ad

pylafleur temporarily deployed to github-pages January 22, 2025 16:47 — with GitHub Actions Inactive

pylafleur temporarily deployed to github-pages January 22, 2025 16:48 — with GitHub Actions Inactive

meriouma reviewed Feb 11, 2025

View reviewed changes

	Object.entries(theme.alias).forEach(([contextualTokenName, tokenValue]) => {
	Object.entries(theme.alias).forEach(([maybeContextualTokenName, tokenValue]) => {

Uh oh!

Conversation

pylafleur commented Jan 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jan 13, 2025

Uh oh!

github-actions Bot commented Jan 13, 2025

Uh oh!

Uh oh!

Uh oh!

savutsang Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pylafleur commented Jan 22, 2025

Uh oh!

meriouma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pylafleur Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

savutsang Jan 16, 2025 •

edited

Loading

pylafleur Feb 11, 2025 •

edited

Loading