-
Notifications
You must be signed in to change notification settings - Fork 7
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
[Unités] Affichage et édition des unités sur la carte #2716
Conversation
6b753d2
to
39ea4b3
Compare
39ea4b3
to
156e200
Compare
frontend/src/api/constants.ts
Outdated
export const ARCHIVE_GENERIC_ERROR_MESSAGE = 'An unexpected error occurred while attempting to archive this entity.' | ||
export const DELETE_GENERIC_ERROR_MESSAGE = 'An unexpected error occurred while attempting to delete this entity.' | ||
|
||
export const RTK_DEFAULT_QUERY_OPTIONS = { |
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.
Je n'appellerais pas ça comme ça ^^
C'est pas une option par défaut, au contraire, je préfère expliciter ce polling.
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.
Je l'ai appelé RTK_COMMON_QUERY_OPTIONS
mais je ne sais pas si ça te suffit donc je laisse ouvert.
C'est que rallonger le code à chaque fois avec des options qui sont généralement les mêmes me paraît too much. On peut aussi renommer encore plus précisément en RTK_5_MINUTES_POLLING_OPTIONS.
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.
Le polling n'est pas utilisé utilisé partout dans le code et c'est 1 ligne de config, à la limite RTK_5_MINUTES_POLLING_OPTIONS
est déjà plus explicite mais je trouve que ça challenge un peu cette variable...
onConfirm: () => Promisable<void> | ||
title: string | ||
} | ||
export function ConfirmationModal({ |
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.
Parfois on utilise Dialog
et parfois Modal
, pourquoi ?
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.
https://www.google.com/search?q=dialog+vs+modal ^^
En bref :
- Une modal oblige l'utilisateur à faire un choix (confirmer, annuler, etc). Il ne peut pas l'ignorer en cliquant sur une croix de fermeture de fenêtre ou en cliquant à l'extérieur.
- Une dialog est un simple message informatif affiché à l'utilisateur qui peut être ignoré (bouton "fermer", "X" ou clic extérieur pour la fermer généralement).
|
||
// TODO Allow direct `width` prop control in MUI. | ||
// This is a mess. I wonder if we should add inner classes in MUI. | ||
const StyledDialog = styled(MuiDialog)<{ |
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.
là on redéfinit le style de base de MonitorUI ? C'est la charte gloale Fish/Env ?
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.
Je checkerai avec Adeline alors mais c'est le style utilisé sur Env oui.
const dispatch = useMainAppDispatch() | ||
const controlUnitId = useMainAppSelector(store => store.controlUnitDialog.controlUnitId) | ||
if (!controlUnitId) { | ||
throw new FrontendError('`mapControlUnitDialog.controlUnitId` is undefined.') |
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.
Je suis toujours un peu mitigé sur ces erreurs qui pop à l'utilisateur final...
import { useMainAppDispatch } from '../../hooks/useMainAppDispatch' | ||
import { useMainAppSelector } from '../../hooks/useMainAppSelector' | ||
|
||
// TODO DRY that with `frontend/src/features/MapButtons/shared/MapToolButton.tsx` when we'll migrate to using MUI-only components. |
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.
Je ne sais pas s'il faut laisser ce Dialog
"à la Env" ici, au lieu d'utiliser notre MapToolButton
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.
Je checkerai aussi avec Adeline mais il me semble bien qu'on commence à harmoniser les deux.
156e200
to
5824105
Compare
837679e
to
4fce8df
Compare
66e5e1a
to
d0a3ee7
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Je merge en gardant les comments non résolus ouverts pour suivi sur les prochaines PRs. |
@@ -33,7 +33,9 @@ export function HealthcheckHeadband() { | |||
} | |||
|
|||
if (isError || !healthcheck) { | |||
dispatch(setError(error)) | |||
if (error) { |
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.
Si isError
n'est pas utilisable, autant l'enlever non ?
Linked issues