-
Notifications
You must be signed in to change notification settings - Fork 1
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
[Couches Reg & AMP] Zoom sur les groupes de couche #1168
Conversation
89a12f4
to
855e138
Compare
import fr.gouv.cacem.monitorenv.domain.entities.regulatoryArea.RegulatoryAreaEntity | ||
import org.locationtech.jts.geom.MultiPolygon | ||
|
||
data class RegulatoryAreasDataOutput( |
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.
on était pas parti sur du singulier pour les entities/output/input ?
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.
Oui tu as raison c'est pas top ce pluriel. J'avais fait au début un RegulatoryAreasDataOutput
et un RegulatoryAreaDataOutput
pour différencier quand on renvoie plusieurs regulatoryArea (avec moins de données) ou un seul avec toutes les données mais ce n'est pas clair (On a par exemple dans le même style pas très explicite ReportingDataOutput
et ReportingsDataOuput
). Là j'ai fait un RegulatoryAreaWithMetadataDataOutput
que je trouve plus clair quand je renvoie toutes les données. Pas évident ce nommage de sorties différentes pour un même objet. Je ne suis pas fan des compact
full
complete
.
@@ -51,8 +51,12 @@ data class RegulatoryAreaModel( | |||
val date_fin: String?, | |||
@Column(name = "temporalite") | |||
val temporalite: String?, | |||
@Column(name = "action") | |||
val action: String?, |
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.
il me semble qu'on avait dit qu'on les triais par ordre alphabétique
// TODO Migrate this middleware. | ||
export const regulatoryLayersErrorLoggerMiddleware: Middleware = store => next => action => { | ||
if (regulatoryLayersAPI.endpoints.getRegulatoryLayers.matchRejected(action)) { | ||
store.dispatch(setToast({ message: "Nous n'avons pas pu récupérer les Zones Réglementaires" })) |
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.
A rediscuter en point dev je pense, parce que ca fait une deuxième façon de gérer les erreurs. Ivan avait commencer une proposition ici (https://github.com/MTES-MCT/monitorfish/wiki/Gestion-des-erreurs-%5BProposition%5D). Et avait commencer à gérer les erreurs comme ici : https://github.com/MTES-MCT/monitorenv/blob/main/frontend/src/api/stationsAPI.ts#L20
Ce sera dommage qu'on ai plusieurs façon de faire
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.
ah cool j'avais pas vu.
frontend/src/api/ampsAPI.ts
Outdated
} | ||
|
||
return acc | ||
}, {}) |
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.
a typer ?
color={ampZonesAreShowed ? THEME.color.blueGray : THEME.color.slateGray} | ||
data-cy={ampZonesAreShowed ? 'amp-my-zones-zone-hide' : 'amp-my-zones-zone-show'} | ||
color={ampZonesAreShowed ? THEME.color.charcoal : THEME.color.lightGray} | ||
data-cy={ampZonesAreShowed ? 'my-amp-zones-zone-hide' : 'my-amp-zones-zone-show'} |
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.
doublon du mot zone
title={ampZonesAreShowed ? 'Cacher la/les zone(s)' : 'Afficher la/les zone(s)'} | ||
/> | ||
|
||
<IconButton | ||
accent={Accent.TERTIARY} | ||
data-cy="amp-layers-my-zones-zone-delete" | ||
color={THEME.color.lightGray} | ||
data-cy="my-amp-layers-zones-zone-delete" |
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.
doublon du mot zone
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.
en testant je vois bien que ca zoome correctement quand on clique sur un groupe de zones dans Mes Amps
ou Mes zones réglementaires
. Par contre je trouve bizarre la double utilisation de l'oeil une fois pour afficher ou non et une autre fois pour zoomer (ça oblige, pour zoomer, à cliquer deux fois sur l'oeil). C'est plus une remarque générale qui n'a pas forcément de lien avec cette pr, peut-être en parler avec Adeline
Dans le ticket Adeline demande à faire la même chose pour les résultats de recherche, j'ai pas l'impression que ça ai été fait (#426 (comment))
export function AMPLayersList() { | ||
const selectedAmpLayerIds = useAppSelector(state => state.selectedAmp.selectedAmpLayerIds) | ||
const showedAmpLayerIds = useAppSelector(state => state.selectedAmp.showedAmpLayerIds) | ||
|
||
const { currentData: amps, isLoading } = useGetAMPsQuery() | ||
const selectedAmps = useMemo( | ||
() => selectedAmpLayerIds.map(id => amps?.entities?.[id]).filter(l => l), | ||
() => selectedAmpLayerIds.map(id => amps?.entities?.[id]).filter((l): l is AMP => !!l), |
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.
layer
à la place de l
qui n'est pas très compréhensible fonctionnellement
|
||
const toggleLayerDisplay = e => { | ||
e.stopPropagation() | ||
if (regulatoryZonesAreShowed) { | ||
dispatch(hideRegulatoryLayers(groupLayerIds)) | ||
} else { | ||
const extentOfGroupLayers = layers.reduce((accumulatedExtent, currentLayer) => { |
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.
j'ai l'impression que c'est la même fonction que pour les amp, on peut peut-être la sortir dans un fichier?
|
||
const clickOnGroupZones = () => { | ||
setZonesAreOpen(!zonesAreOpen) | ||
if (typeof clearSelectedLayer === 'function') { |
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.
ca ne suffit pas if (clearSelectedLayer) {}
?
@@ -34,7 +35,7 @@ export function LayerSearch({ isVisible }) { | |||
|
|||
const [filterSearchOnMapExtent, setFilterSearchOnMapExtent] = useState<boolean>(false) | |||
|
|||
const isSearchThrottled = useRef(false) | |||
// const isSearchThrottled = useRef(false) |
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.
a supprimer ?
@@ -66,7 +61,7 @@ export function LayerSearch({ isVisible }) { | |||
threshold: 0.2 | |||
}) | |||
|
|||
return async ({ | |||
const searchFunction = async ({ |
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.
il n'y aurait pas moyen de découper cette fonction, qu'elle soit un peu plus lisible ?
}, [dispatch, regulatoryLayers, amps]) | ||
|
||
// const debouncedSearchLayers = useCallback(_.debounce(memoizedSearchFunction, 1300, { trailing: true }), []) |
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.
a supprimer ?
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.
désolé..
.../main/kotlin/fr/gouv/cacem/monitorenv/domain/entities/regulatoryArea/RegulatoryAreaEntity.kt
Show resolved
Hide resolved
4e090c9
to
543046f
Compare
543046f
to
6a02045
Compare
enforce debouncedSearch on trailing event add more typing rename dataoutput for regulatoryAreas
add typing refacto getExtentOfLayersGroup
fix L'œil pour visualiser un groupe de zones épinglées ne centre pas les zones sur la carte #426
6a02045
to
97c945c
Compare
resolve #426