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

[Points d'intérêt] – Supprimer les types de points, pouvoir masquer les points sur la carte, changer l'icone #1397

Merged
merged 14 commits into from
May 27, 2024

Conversation

maximeperraultdev
Copy link
Collaborator

Related Pull Requests & Issues


  • Tests E2E (Cypress)

What have I done in this PR

  • Refacto EditInterestPoint to use the same component than Mission's
  • Rewriting test to make e2e cypress tests idempotent
  • Upgrade cypress to 13.10.0 because of a bug when several windows are open
  • A little bit of BSR (Boy Scout Rules) : typo, extract methods for lisibility, small css fixes

@maximeperraultdev maximeperraultdev added the feat. enhancement Amélioration/évolution d'une fonctionnalité label May 23, 2024
@maximeperraultdev maximeperraultdev self-assigned this May 23, 2024
@maximeperraultdev maximeperraultdev requested review from claire2212, ivangabriele and thoomasbro and removed request for ivangabriele May 23, 2024 13:02
cy.wait('@createControlUnit').then(interception => {
if (!interception.response) {
cy.wait('@createControlUnit').then(({ request, response }) => {
if (!response) {
assert.fail('`interception.response` is undefined.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

message à modifier: juste response is undefined ?

}
cy.wait('@updateControlUnit').then(({ request: requestOfUpdate, response: responseOfUpdate }) => {
if (!responseOfUpdate) {
assert.fail('`interception.response` is undefined.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

idem message

cy.wait('@createStation').then(interception => {
if (!interception.response) {
cy.wait('@createStation').then(({ request, response }) => {
if (!response) {
assert.fail('`interception.response` is undefined.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

idem message

@@ -199,39 +202,52 @@ context('Side Window > Mission Form > Main Form', () => {
cy.get('*[data-cy="delete-mission"]').should('be.disabled')
})

it('A user can delete mission if control unit already engaged and be redirected to filtered mission list', () => {
it('An user can cancel mission creation if control unit already engaged and be redirected to filtered mission list', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

pour moi c'est bien A user non, même s'il y a une voyelle?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Capture d’écran 2024-05-24 à 10 11 43 Non, c'est bien "an user"

cy.wait('@getEngagedControlUnits')
cy.fill('Période', 'Période spécifique')
cy.fill('Période spécifique', [expectedStartDate, expectedEndDate])
const unite = 'PAM Jeanne Barret'
Copy link
Collaborator

Choose a reason for hiding this comment

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

on les nomme plutôt en controlUnit dans le code les unités

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 la force de l'habitude du français

@@ -51,6 +53,8 @@ export type BaseMapChildrenProps = {
pixel: number[] | undefined
}

type VectorLayerType = VectorLayerWithName & VectorLayer<VectorSource>
Copy link
Collaborator

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 : type VectorLayerType = VectorLayerWithName | VectorLayer<VectorSource> ? Parce le layer des points d'intérêt n'est pas de type VectorLayerWithName

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Au final on peut supprimer ce type car on ne l'utilise VectorLayer. Et je t'avoue que je ne sais plus pourquoi on l'avais créé au départ.

Copy link
Collaborator

Choose a reason for hiding this comment

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

on avait changé, car le layer des point d'intérêt n'est pas de type VectorLayerWithName mais de type VectorLayer<VectorSource> , donc ça m'étonne qu'on puisse le supprimer. Mais si ça fonctionne ok

@@ -338,4 +328,42 @@ export function InterestPointLayer({ map }: BaseMapChildrenProps) {
) : null}
</div>
)

function shouldStyledLines(feature) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

comme la fonction renvoie un booléen tu pourrais la préfixer par is, genre isLineFeature ou getisLineFeature, t'en penses quoi? on essaye d'avoir une convention pour les booléen (variables et fonctions).
Je comprends bien ta façon de nommer mais sur env on part plutôt sur l'idée que le nom d'une fonction reflète ce qu'elle fait/ doit renvoyer et non pourquoi elle est appelée (ici t'es plutôt parti la dessus on dirait)

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 nomme les fonctions qui renvoie des booléens avec des "yes/no questions" (si tu te rappelles de tes cours d'anglais du collège). Donc j'ai tendance à nommer ces méthodes avec des is/has/should. Après si c'est une convention c'est pas un pb

Copy link
Collaborator

@claire2212 claire2212 May 24, 2024

Choose a reason for hiding this comment

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

Ce que je veux dire c'est que la fonction te permet de savoir si la feature est une ligne ou non, pas de savoir si on doit la stylisée. Si cette fonction était utilisée ailleurs que pour le style (par exemple savoir si on affiche ou non un truc si la feature est une ligne) le nom de la fonction ne serait plus très pertinent, d'où ma proposition de renommage en isLineFeature , tu vois ce que je veux dire?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes je vois, je modifie 👍


(uuid: string) => {
removePoint(uuid)
removeLine(uuid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

pourquoi faire deux fonctions?

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 trouve que ça gagne en lisibilité

return 'Point_interet_feature_autre.png'
}
}
export const getInterestPointStyle = (resolution: number) => getIconStyle(resolution)
Copy link
Collaborator

@claire2212 claire2212 May 24, 2024

Choose a reason for hiding this comment

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

je suis pas sûre de comprendre pourquoi ajouter une fonction qui ne fait qu'appeler une autre fonction

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

getInterestPointStyle est exporter et l'autre non. (elle l'est car j'avais fait des tests sur le style mais au final ce n'est plus utile).
Je supprime le export de getIconStyle et c'est bon pour toi ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

tu peux pas supprimer la fonction getInterestPointStyle plutôt ?

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 découpler la récupération style d'un point d'interet et son style "réel" (une icone)

@maximeperraultdev maximeperraultdev force-pushed the maxime/feat/1214/point_d_interet branch from 004b807 to 9c6a97d Compare May 27, 2024 11:56
@maximeperraultdev maximeperraultdev force-pushed the maxime/feat/1214/point_d_interet branch from 9c6a97d to 70a14aa Compare May 27, 2024 12:12
@maximeperraultdev maximeperraultdev force-pushed the maxime/feat/1214/point_d_interet branch from 70a14aa to fdab195 Compare May 27, 2024 12:15
@maximeperraultdev maximeperraultdev merged commit 9fdd0ec into main May 27, 2024
21 checks passed
@maximeperraultdev maximeperraultdev deleted the maxime/feat/1214/point_d_interet branch May 27, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat. enhancement Amélioration/évolution d'une fonctionnalité
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Points d'intérêt – Amélioration de la fonctionnalité
3 participants