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

[TECH] Convertit les composants du dossier banner sur PixOrga au format GJS (PIX-12532). #8896

Merged
merged 4 commits into from
May 22, 2024

Conversation

frinyvonnick
Copy link
Member

@frinyvonnick frinyvonnick commented May 13, 2024

🦄 Problème

Ember Polaris introduit un nouveau format de composant (cf Template Tag Format). L'équipe de développement d'Ember prévoit que ce soit celui par défaut d'ici la fin de cette version majeure.

Le but de cette PR est de montrer ce qu'il est possible de faire avec ce nouveau format et de présenter quelques techniques de migration possible.

🤖 Proposition

Quelques composants ont été choisis parce qu'ils présentaient des caractéristiques intéressantes à migrer :

  • Banner::Communication : migration d'un composant avec des getters
  • Banner::Information : démonstration de la création de composants privés
  • Banner::LanguageAvailability : démonstration de comment se passer d'une classe quand on utilise un service

🌈 Remarques

Les principales sources d'informations sont :

Le but de cette démonstration est aussi de montrer qu'on est plus obligé d'utiliser les classes. C'est d'ailleurs un pré-requis si on ne souhaite pas tout mettre dans le même fichier avec ce nouveau format.

☝️ Je précise que je n'ai pas d'avis sur le sujet je montre juste ce qui est possible.

Le dernier point qui oblige actuellement à utiliser les classes, c'est l'utilisation du decorator tracked. Les Ressources sont sensés nous permettre de nous en passer (c'est le sujet de la prochaine MEJ).

Cette PR pourrait être mergé à part le dernier commit qui est exclusif à la Mise en Jambe (à drop si on souhaite aller plus loin).

💯 Pour tester

La CI est verte ✅

@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@frinyvonnick frinyvonnick force-pushed the test-ember-functions branch 3 times, most recently from 47aa348 to d87e087 Compare May 15, 2024 09:47
@frinyvonnick frinyvonnick marked this pull request as ready for review May 15, 2024 09:47
@frinyvonnick frinyvonnick requested a review from a team as a code owner May 15, 2024 09:47
@frinyvonnick
Copy link
Member Author

Après discussion, la @1024pix/team-acces souhaite potentiellement merger cette PR réalisé dans le cadre d'une MEJ. J'ai donc droppé le commit qui ajoutait une page pour la démonstration et passer la PR en ready to review.

Le commit d87e087 est un peu opnionated et sera peut être remis en cause si on décide d'utiliser les Resources en early adopters. Je vous laisse juger de la pertinence de le garder ou pas 😄

@frinyvonnick frinyvonnick changed the title [TECH] PR pour faire la MEJ sur le format de composant avec la balise template [TECH] Converti les composants du dossier banner sur PixOrga au format GJS. May 15, 2024
@frinyvonnick frinyvonnick changed the title [TECH] Converti les composants du dossier banner sur PixOrga au format GJS. [TECH] Convertit les composants du dossier banner sur PixOrga au format GJS. May 15, 2024
@frinyvonnick frinyvonnick changed the title [TECH] Convertit les composants du dossier banner sur PixOrga au format GJS. [TECH] Convertit les composants du dossier banner sur PixOrga au format GJS (PIX-12532). May 15, 2024
Copy link
Contributor

@Alexandre-Monney Alexandre-Monney left a comment

Choose a reason for hiding this comment

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

C'est beau 🤩 🐈‍⬛

import { getOwner } from '@ember/application';
import Helper from '@ember/component/helper';

export default class GetService extends Helper {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion : rajouter un bout de jsdoc qui montre comment on s'en sert, j'ai l'impression que la syntaxe let .. as |value| et pas forcément très connu

Copy link
Member Author

Choose a reason for hiding this comment

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

C'est un des helpers mis en avant dans la documentation officielle : https://guides.emberjs.com/release/components/helper-functions/#toc_the-let-helper

Copy link
Member Author

Choose a reason for hiding this comment

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

La liste exhaustif des helpers de ember sont là : https://api.emberjs.com/ember/4.5.0/classes/Ember.Templates.helpers/

Copy link
Contributor

Choose a reason for hiding this comment

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

Le let je ne l'ai jamais utilisé, et le as |value| je l'ai plus utilisé pour des boucles for. Mais oui, un peu de documentation ferait du bien.

Copy link
Member Author

Choose a reason for hiding this comment

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

as |value| est la syntaxe pour définir une variable dans le template à partir d'une source de données. C'est le cas effectivement des boucles mais c'est aussi utilisé pour les yieldable block qui ont des paramètes (la doc : https://guides.emberjs.com/v3.12.0/components/block-params/).

Copy link
Member Author

Choose a reason for hiding this comment

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

Il y a ici un exemple avec la définition de plusieurs variables : https://guides.emberjs.com/v3.12.0/templates/displaying-a-list-of-items/#toc_accessing-an-items-index

Comment on lines +21 to +22
{{#if (isEnabled)}}
<PixBanner @type={{(bannerType)}} class="sticker-banner">
Copy link
Contributor

Choose a reason for hiding this comment

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

Si je comprends bien, on est maintenant obligé d'utiliser des parenthèses (NOM_DE_FONCTION) dans les nouveaux templates ? Je suppose pour spécifier que l'on n'utilise pas de classe pour le composant.

Copy link
Member Author

Choose a reason for hiding this comment

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

La syntaxe avec les parenthèse permet d'appeler les fonctions. C'est obligatoire quand il y a des paramètres, je peux vérifier si ici on peut s'en passer 😄

}

<template>
{{#let (getService "service:session") as |session|}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Serait-on obligé de faire ceci pour une liste de services que l'on souhaite importer ? Si oui, c'est assez verbeux. N'y a-t-il pas d'autre moyen d'importer un ou plusieurs services pour un composant ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Comme dit sur Slack, cette manière de faire risque d'évoluer avec l'utilisation des Ressource. On a accès directement au owner dans les Ressources donc on peut récupérer facilement les services dedans. Ici c'était une proposition pour pouvoir se passer des classes afin de pouvoir mettre le code JavaScript dans des différents du code de templating.

A titre personnel, je n'ai pas d'avis (les deux solutions me vont classe ou fonction). J'avais d'ailleurs proposé de dropper le commit 😉

<PixBanner
@type="information"
@canCloseBanner="true"
@onCloseBannerTriggerAction={{fn session.updateDataAttribute "localeNotSupportedBannerClosed" true}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Si on souhaite rendre, plus lisible ce code (de mon point de vue), on pourrait faire ceci et remettre la fonction closeBanner avec un argument session, c'est bien ça ?

Suggested change
@onCloseBannerTriggerAction={{fn session.updateDataAttribute "localeNotSupportedBannerClosed" true}}
@onCloseBannerTriggerAction={{(closeBanner session)}}

Copy link
Member Author

Choose a reason for hiding this comment

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

Oui, c'était mon implémentation initiale. Mais je voulais montrer ici que ça n'est pas obligatoire il y a tous les outils dans Ember par défaut pour faire autrement 😄

import { getOwner } from '@ember/application';
import Helper from '@ember/component/helper';

export default class GetService extends Helper {
Copy link
Contributor

Choose a reason for hiding this comment

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

Le let je ne l'ai jamais utilisé, et le as |value| je l'ai plus utilisé pour des boucles for. Mais oui, un peu de documentation ferait du bien.

Copy link
Contributor

@xav-car xav-car left a comment

Choose a reason for hiding this comment

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

je suis plutôt pour drop le dernier commit histoire d'avoir une certaine consistance dans la déclaration de nos composants.

Mais si on laisse comme ça, ça me va aussi. ( réponse de Normand )

@pix-service-auto-merge pix-service-auto-merge merged commit 9db44a4 into dev May 22, 2024
5 of 7 checks passed
@pix-service-auto-merge pix-service-auto-merge deleted the test-ember-functions branch May 22, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants