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

refactor: cleaned alert component #54

Merged
merged 1 commit into from
Mar 7, 2025
Merged

refactor: cleaned alert component #54

merged 1 commit into from
Mar 7, 2025

Conversation

LAMM26
Copy link
Collaborator

@LAMM26 LAMM26 commented Jan 6, 2025

Nettoyage du composant, renommage des classes.

@LAMM26 LAMM26 self-assigned this Jan 6, 2025
<div [class]="getAlertClass()" [class.--is-handset]="isHandset()">
<mat-icon [class]="getIconClass()">{{ iconName() }}</mat-icon>
<div class="message" [class.--is-handset]="isHandset()">
<div class="sdg-alert-container" [class]="getAlertClass()">
Copy link
Contributor

Choose a reason for hiding this comment

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

C'est quoi le plus value pour ce tag container?
Si on est capable d'utiliser le host au lieu d'un tag en extra on est mieux d'utiliser le host.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Je crois qu'il est nécessaire pour appliquer la classe .--generalou .--warning (le background-color)

Copy link
Contributor

@alecarn alecarn left a comment

Choose a reason for hiding this comment

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

Je vais pousser un commit avec ce que je pense l'idéal, tu vérifieras si c'est bien correct pour la structure et la suppression du containerClass

<mat-icon [class]="getIconClass()">{{ iconName() }}</mat-icon>
<div class="message" [class.--is-handset]="isHandset()">
<div class="sdg-alert-container" [class]="typeClass()">
<div [class]="containerClass()">
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi le containerClass?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

C'est pour que les marges dans l'alerte soient égales au reste:

image

Copy link
Contributor

@alecarn alecarn Mar 7, 2025

Choose a reason for hiding this comment

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

J'ai reverté une partie, pour remettre le containerClass

styleUrls: ['./alert.component.scss']
styleUrls: ['./alert.component.scss'],
host: {
'[class]': 'typeClass()'
Copy link
Contributor

@alecarn alecarn Mar 7, 2025

Choose a reason for hiding this comment

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

@LAMM26 , on peut utiliser le host pour ajouter, entre autres, des classes. Ça l'évite d'ajouter des tags a nos composantes et de complexifier la structure du dom.

@LAMM26 LAMM26 reopened this Mar 7, 2025
@LAMM26 LAMM26 merged commit 81f88ab into next Mar 7, 2025
4 checks passed
@LAMM26 LAMM26 deleted the fix/alert branch March 7, 2025 18:29
@alecarn
Copy link
Contributor

alecarn commented Mar 7, 2025

🎉 This PR is included in version 1.0.0-next.50 🎉

The release is available on:

Your semantic-release bot 📦🚀

@LAMM26 LAMM26 restored the fix/alert branch March 7, 2025 18:47
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.

2 participants