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

Cleanup #51

Merged
merged 18 commits into from
Mar 18, 2024
Merged

Cleanup #51

merged 18 commits into from
Mar 18, 2024

Conversation

pelord
Copy link
Member

@pelord pelord commented Jan 25, 2024

Please check if the PR fulfills these requirements

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[ ] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications:

Other information:

@pelord pelord marked this pull request as draft January 25, 2024 22:19
@alecarn alecarn requested review from mneige, LAMM26 and alecarn January 26, 2024 14:55
Copy link
Collaborator

@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.

Good job @pelord 💪

src/app/app.component.ts Outdated Show resolved Hide resolved
src/environments/environment.ts Outdated Show resolved Hide resolved
src/app/app.component.ts Show resolved Hide resolved
CoordinatesSearchResultFormatter,
provideDefaultCoordinatesSearchResultFormatter(),
provideDefaultIChercheSearchResultFormatter(),
provideSearchSourceService(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tous les autres providers n'étaient pas utiliser?

Copy link
Member Author

Choose a reason for hiding this comment

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

j'ai repris la syntaxe de l'assemblage.. igo2.. n'étant plus la, je me suis dit qu'ils n'étaient plus nécessaires!

src/app/app.module.ts Outdated Show resolved Hide resolved
@@ -66,7 +60,6 @@ export const defaultTooltipOptions: MatTooltipDefaultOptions = {
IgoSpinnerModule,
IgoStopPropagationModule,
PortalModule,
HammerModule,
HeaderModule,
FooterModule,
Copy link
Collaborator

Choose a reason for hiding this comment

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

FooterModule et GestureModule, est-ce qu'ils sont nécessaire?

Copy link
Member Author

Choose a reason for hiding this comment

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

aucune idée pour FooterModule

Copy link
Member Author

Choose a reason for hiding this comment

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

@mneige peux tu valider?

Copy link
Contributor

@mneige mneige Feb 7, 2024

Choose a reason for hiding this comment

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

Footermodule oui pour faire un footer de la map, c'est une config. Le footer est demandé par le système de design. Gesture c'était pour certains comportements mobiles

@alecarn
Copy link
Collaborator

alecarn commented Jan 26, 2024

@pelord, j'ai remarqué une erreur dans VsCode par rapport au MenuModule qui me mène vers le MapComponent. Cette composante est déclarée dans deux modules, MapModule et MenuModule. On pourrait changer la déclaration du MenuModule pour qu'il importe le MapModule à la place

CoordinatesSearchResultFormatter,
provideDefaultCoordinatesSearchResultFormatter(),
provideDefaultIChercheSearchResultFormatter(),
provideSearchSourceService(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Il y a la méthode setWorkspaces de SearchSourceService qui est en erreut. Il faudrait le conserver?
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Tu as raison. J'ai pas poussé mon fixe dans la lib encore..mais ça va être ok avec le fix. Bien vu

Copy link
Member Author

Choose a reason for hiding this comment

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

@pelord
Copy link
Member Author

pelord commented Jan 26, 2024

@LAMM26 @mneige @alecarn
Pourquoi il y a un duplicat du searchState? Pourquoi ne pas utiliser celui de la lib?

@alecarn
Copy link
Collaborator

alecarn commented Jan 29, 2024

Pourquoi il y a un duplicat du searchState? Pourquoi ne pas utiliser celui de la lib?

Bonne question, s'il n'y a pas de différence dans la logique d'affaire on pourrait le supprimer et remplacer par celui de la lib.

Pour ton info, on a fait certaine analyse #44 et #46 sur ce type de divergence. Malheureusement les conclusions ne sont pas documenté mais dans certain cas, on a décidé de conserver les divergences vu qu'il y avait des différences dans la logique d'affaire

Copy link
Contributor

@mneige mneige left a comment

Choose a reason for hiding this comment

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

Quand la légende ne s'affiche pas dans le panneau latéral ( "legendInPanel": false,) elle a perdu son positionnement. Elle est maintenant centrée au lieu d'être au-dessus des boutons de la carte. On laisse comme ça?

@mneige
Copy link
Contributor

mneige commented Feb 7, 2024

@LAMM26 @mneige @alecarn Pourquoi il y a un duplicat du searchState? Pourquoi ne pas utiliser celui de la lib?

Simplement pour pouvoir le personnaliser mais on peut pointer vers la lib, c'est au besoin. Ou y ajouter des configs dans la lib

Base automatically changed from code-formatting to next March 14, 2024 15:37
@pelord pelord marked this pull request as ready for review March 18, 2024 18:55
@pelord pelord merged commit 46bb652 into next Mar 18, 2024
1 check passed
@pelord pelord deleted the cleanup branch March 18, 2024 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants