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.

Sign up for GitHub

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

Jump to bottom

Préavis – Consulter le détail des signalements à partir des tags signalements de la liste #3813

Merged
merged 24 commits into from
Nov 11, 2024
Merged

Préavis – Consulter le détail des signalements à partir des tags signalements de la liste #3813

merged 24 commits into from
Nov 11, 2024

Conversation

ivangabriele
Copy link
Member

@ivangabriele ivangabriele commented Oct 31, 2024 •
edited
Loading

@ivangabriele ivangabriele added bug Something isn't working feat. enhancement feature enhancement labels Oct 31, 2024
@ivangabriele ivangabriele force-pushed the ivan/open-reporting-list-from-tags-in-side-window-pno-list branch 3 times, most recently from 2bd0d92 to b0663db Compare November 5, 2024 17:23
@ivangabriele ivangabriele marked this pull request as ready for review November 6, 2024 12:14
@ivangabriele ivangabriele force-pushed the ivan/open-reporting-list-from-tags-in-side-window-pno-list branch from 9cbb3d3 to c13ac07 Compare November 6, 2024 12:18
louptheron
louptheron reviewed Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@louptheron louptheron left a comment

Choose a reason for hiding this comment

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

Quelques commentaires et questions

...src/features/Logbook/components/VesselLogbook/LogbookMessages/messages/common/SpecyCatch.tsx Outdated Show resolved Hide resolved
...src/features/Logbook/components/VesselLogbook/LogbookMessages/messages/common/SpecyCatch.tsx Outdated Show resolved Hide resolved
...src/features/Logbook/components/VesselLogbook/LogbookMessages/messages/common/SpecyCatch.tsx Show resolved Hide resolved
frontend/src/features/PriorNotification/components/ManualPriorNotificationForm/Content.tsx Outdated
@@ -46,6 +47,9 @@ export function Content({ detail, isValidatingOnChange, onClose, onSubmit, onVer

const [isCancellationConfirmationModalOpen, setIsCancellationConfirmationModalOpen] = useState(false)
const [isInvalidationConfirmationModalOpen, setIsInvalidationConfirmationModalOpen] = useState(false)
const [selectedVesselIdentity, setSelectedVesselIdentity] = useState<VesselIdentity | undefined>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pourquoi utiliser useState et pas simplement un const ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Parce que cette valeur change à chaque fois qu'on sélectionne un nouveau navire, elle est ensuite passée à l'entête pour le mettre à jour, Ainsi que la première fois en tant que valeur initiale pour le champ FormikVesselSelect ce qui permet d'éviter un appel API supplémentaire à l'ouverture du formulaire.

J'ai fait ça pour contribuer à l'amélioration des performances d'ouverture d'un formulaire manuel.

frontend/src/features/PriorNotification/components/ManualPriorNotificationForm/Form.tsx Outdated Show resolved Hide resolved
...tures/PriorNotification/components/ManualPriorNotificationForm/fields/FormikVesselSelect.tsx Outdated Show resolved Hide resolved
frontend/src/features/Vessel/components/VesselSearch/index.tsx Outdated Show resolved Hide resolved
frontend/src/features/PriorNotification/components/ManualPriorNotificationForm/Content.tsx Outdated
detail={detail}
isNewPriorNotification={isNewPriorNotification}
onClose={handleClose}
selectedVesselIdentity={selectedVesselIdentity}
Copy link
Collaborator

Choose a reason for hiding this comment

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

selectedVesselIdentity n'était pas déjà dans detail ?

Copy link
Member Author

Choose a reason for hiding this comment

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

selectedVesselIdentity peut être différent de detail.vesselIdentity lorsqu'on édite un préavis en modifiant le navire existant.

frontend/src/features/PriorNotification/components/ManualPriorNotificationForm/Form.tsx Outdated Show resolved Hide resolved
...tures/PriorNotification/components/ManualPriorNotificationForm/fields/FormikVesselSelect.tsx Outdated Show resolved Hide resolved
louptheron
louptheron reviewed Nov 7, 2024
View reviewed changes
...tures/PriorNotification/components/ManualPriorNotificationForm/fields/FormikVesselSelect.tsx Outdated
onChange: (nextVessel: VesselIdentity | undefined) => void
readOnly?: boolean | undefined
}>
export function FormikVesselSelect({ onChange, readOnly }: FormikVesselSelectProps) {
const defaultValueRef = useRef<VesselIdentity | undefined>(undefined)
export function FormikVesselSelect({ initialVesselIdentity, onChange, readOnly }: FormikVesselSelectProps) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

On peut l'enlever vu qu'il y a le vesselId ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Voir le commentaire sur l’optimisation des appels.

@ivangabriele ivangabriele force-pushed the ivan/open-reporting-list-from-tags-in-side-window-pno-list branch from c13ac07 to b2d3458 Compare November 7, 2024 17:34
louptheron
louptheron reviewed Nov 8, 2024
View reviewed changes
frontend/src/features/Vessel/useCases/searchVessels.ts
const queryParams = {
searched: searchQuery.toUpperCase()
}
const foundVesselsFromApi = await dispatch(vesselApi.endpoints.searchVessels.initiate(queryParams)).unwrap()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Avec un forceRefetch ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ce n'est pas plutôt peu fréquent la mise à jour de la liste des navire dans la DB ? La pipeline les met à jour tous les combien ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oui c'est vrai que c'est toutes les 3h, j'avais peur de rajouter encore du délai (car les utilisateurs laissent leur appli ouverte toute la journée) maius on peut essayer comme ça !

louptheron
louptheron reviewed Nov 8, 2024
View reviewed changes
frontend/src/features/Vessel/components/VesselSearch/index.tsx
@@ -28,6 +26,7 @@ export type VesselSearchProps = Readonly<
onChange: (nextVessel: VesselIdentity | undefined) => Promisable<void>
onFocus?: () => Promisable<void>
onVesselLinkClick?: (vessel: VesselIdentity) => Promisable<void>
shouldCloseOnClickOutside?: boolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pas besoin de props je pense, il faut toujours close au clic en dehors

Copy link
Member Author

@ivangabriele ivangabriele Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Y compris sur la carte ? Parce que sur la carte il me semble que c'est "réduit" un peu largeur vers la droite quand on clique dehors (y compris avec les résultats affichés).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah oui bien vu !

louptheron
louptheron reviewed Nov 8, 2024
View reviewed changes
frontend/src/features/PriorNotification/components/ReportingList.tsx
)
const isReportingFormDirty = useMainAppSelector(store => store.priorNotification.isReportingFormDirty)
const vesselIdentity = useMainAppSelector(store => store.priorNotification.openedReportingListVesselIdentity)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Je ne me souviens plus, pourquoi on a besoin de stocker ça dans redux ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Parce que la liste des signalements est ouverte à partir de 3 endroits différents ce qui serait micmac à gérer via des props si c'est bien ta question ? Et vesselIdentity est ce qui sert à la fois au composant partagé <CurrentReportingList /> et au <CardHeader /> qui sont utilisés dans ce composant.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
24 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@ivangabriele ivangabriele merged commit c2183c2 into master Nov 11, 2024
29 checks passed
@ivangabriele ivangabriele deleted the ivan/open-reporting-list-from-tags-in-side-window-pno-list branch November 11, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@louptheron louptheron louptheron left review comments

Assignees
No one assigned
Labels
bug Something isn't working feat. enhancement feature enhancement
Projects
None yet
Milestone
No milestone
2 participants
@ivangabriele @louptheron