-
Notifications
You must be signed in to change notification settings - Fork 51
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] Passage de PixCertif vers Ember Octane (3.15) #976
Conversation
2de0421
to
ed7e9e7
Compare
Yeah ! Je voulais le faire sur |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merci et bravo pour tout le taf !
certif/app/components/routes/authenticated/sessions/details-item.js
Outdated
Show resolved
Hide resolved
certif/app/components/routes/authenticated/sessions/list-items.hbs
Outdated
Show resolved
Hide resolved
certif/app/components/routes/authenticated/sessions/new-item.hbs
Outdated
Show resolved
Hide resolved
certif/app/components/routes/authenticated/sessions/new-item.js
Outdated
Show resolved
Hide resolved
certif/tests/unit/components/routes/authenticated/sessions/details-item-test.js
Outdated
Show resolved
Hide resolved
...s/unit/components/routes/authenticated/sessions/details/certification-candidates-tab-test.js
Outdated
Show resolved
Hide resolved
certif/tests/unit/components/routes/authenticated/sessions/details/parameters-tab-test.js
Outdated
Show resolved
Hide resolved
certif/tests/unit/components/routes/authenticated/sessions/new-item-test.js
Outdated
Show resolved
Hide resolved
ed7e9e7
to
8f3453d
Compare
I'm deploying this PR to these urls:
Please check it out! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laura-bergoens comme vu ensemble :
certif/app/components/session-finalization-candidates-informations-step.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
1ca2b47
to
91c0fc1
Compare
91c0fc1
to
5beca80
Compare
Just the minimum necessary to pass all tests, except for linter
Except for the ApplicationAdapter, which has a specific mixin mechanism with ember-simple-auth
Except for the routes with mixin from ember-simple-auth
See : https://github.com/emberjs/rfcs/blob/master/text/0481-component-templates-co-location.md It's recommended now to colocate component with their template.
Correcting new syntax in component templates, and turning ember component into glimmer component
We have this weird habbit to create a component that will wrap the template we want to render for a route. This is too bad cause it causes an indirection (and more files created). We can directly define this template and its behaviour at the route level.
Ember community is emphasizing on applying Data-Down, Actions Up pattern. It means that a child component cannot alter a data coming from a parent component. It's also known as unidirectional data flow. We are re-enforcing this pattern at some places where we were altering objects that come from top levels
Wrapping the app is now useless since this RFC from Ember, for more explanation : emberjs/rfcs#280
NotificationsContainer component was already present is parent HBS, so it was redundant
ember-cli-notifications, to its latest version, introduces changes on its CSS. More specifically, it now uses the var() property in CSS. Unfortunately, this property is not supported by IE. Solution for now : we first left an issue on their repo to ask whether or not they plan to support IE soon, and we added a specific css to override native component css for IE
5beca80
to
3a34c38
Compare
🦄 Problème
Ember est officiellement passé sur sa version Octane, avec ses changements de syntaxe, de styles, de procédés, etc...
Ils font ça doucement mais sûrement, et le code se déprécie au fur et à mesure des releases.
Prenons le rythme et transformons PixCertif en une appli Ember Octane
🤖 Solution
J'ai suivi le guide d'upgrade d'Ember vers 3.15. C'est pourquoi le premier commit contient toutes les modifs de package[-lock] nécessaires + quelques modifs à la volée pour faire dans un premier temps passer les tests au vert (sauf les tests de lint, le passage en 3.15 est assez bouleversant !).
Ensuite, j'ai pris un dossier par commit, et refacto le contenu en style Octane.
Normalement chaque commit passe les tests (sauf les premiers commits qui ne passent pas le linter).
Venez-me voir si vous voulez un digest des changements ! 😺
En gros, ce que vous allez voir de façon récurrente :
model.get('attr')
oumodel.set('attr', value
), maintenant c'est à la JSmodel.attr = value
!🌈 Remarques
On utilise le composant ember-cli-notifications pour les notifs. On était en version 4.3.3 jusqu'à présent. Le truc c'est que cette version ne marche plus avec Octane, donc on a aussi monté la version du composant jusqu'à sa latest (à savoir 6.0.0). AUTRE problème suite à ça, le composant ne s'affiche plus sous IE (une histoire de css incompatible avec IE, voir @Krysthalia pour + d'info).
Du coup, la solution en attendant en deux temps :