-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat(alert): add role attribute #668
Conversation
Mmh les tests ne donnent pas les meme resultats en local que sur la build. Sur la build, les packages impactés par les modifications (alert sur summary ou encore popover sur Help) ne donnent pas des snapshots à jour... |
C'est bon pour moi mais les tests ne passent pas sur la CI.. :( |
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.
LGTM
C'est pour ça que je vous ai tagué. Il y a un soucis avec la CI si tu regarde de plus pres. Explication. J'ai mis a jour le Alert ou j'ai ajouté un role. Le snapshot se met a jour avec le role en plus, mais aussi sur le summary qui utilise également le role et donc mon snapshot contient également le role. Mais la CI dit que le summary ne contient pas ce role => Problème. Problème similaire avec le popover et le composant Help. |
hum, cela signifie que le composant Summary ne prend pas la bonne version du composant Alert, c'est bizarre car j'ai vérifié les dependancies du composant Summary, il prend bien la version 3.1.6 du Alert... |
@guillaumechervet @guillaumechervetaxa une idée ? |
8401d47
to
c2edb68
Compare
C'est lerna qui remet les carrets par default, il faut creé une issue et faire un PR pour le configurer pour qu'ils ne mettent pas las caret. |
J'essaye de clone ta branche dans la journée |
Tu peux prendre la master direct. Les snapshots foireront sur ton poste sur le Help. |
a5156f1
to
fd0b57f
Compare
J'ai pris la branche master et je n'ai pas de problème, de même sur ma PR de migration du Help en TS. |
c2edb68
to
11fe9da
Compare
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.
Suggestion : Tu peux séparer la feature et la ci en 2 PR ?
Ben je finir par faire ça. Mais du coup je vais pousser des tests foireux et ca me fait un peut chier mais bon. Vu qu'on trouve pas... |
b64b353
to
0124b65
Compare
J'ai viré les snapshots en question pour le moment |
Le push sur master, je n'ai pas fait exprès, je faisais des tests avec lerna et je ne sais pas comment lerna arrive a bypasser les policies github. |
Bon la Build est KO mais c'est un probleme de soumission à la quality gate (alors qu'on a le retour sonar Ici...) Possible de me valide la PR ? @guillaume-chervet @samuel-gomez @romuleald |
J'ai cliqué sur re-run job depuis github. Il faut aller sur Details, il y a le bouton pour relancer. |
Je l'avais deja fait. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
t'as le clic magique @guillaume-chervet mais je veux bien une validation aussi |
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 @youf-olivier
Add the role Attribute to the alert component to improve accessibility and testing.
(The snapshot for help had updated, it's about a little modification in the commit f4a7ac2)