Skip to content

Conversation

@tonial
Copy link
Contributor

@tonial tonial commented Apr 5, 2024

Pourquoi ?

Pour que ce soit mieux :D

@tonial tonial marked this pull request as draft April 5, 2024 13:36
@notion-workspace
Copy link

@tonial tonial force-pushed the alaurent/breadcrumb_is_dead branch 3 times, most recently from 09c0ea3 to d5f0135 Compare April 10, 2024 03:58
@tonial tonial self-assigned this Apr 10, 2024
@tonial tonial force-pushed the alaurent/breadcrumb_is_dead branch 2 times, most recently from 80aaf3b to 439481b Compare April 11, 2024 08:03
@tonial tonial changed the title [GEN-237] Remplacer le fil d'ariane par des boutons retour Remplacer le fil d'ariane par des boutons retour [GEN-237] Apr 11, 2024
@tonial tonial added the 1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC label Apr 11, 2024
@github-actions
Copy link

🥁 La recette jetable est prête ! 👉 Je veux tester cette PR !


Préciser comment accéder à la fonctionnalité pour le métier :

Copy link
Contributor

@hellodeloo hellodeloo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@tonial tonial marked this pull request as ready for review April 12, 2024 11:58
@tonial tonial requested a review from xavfernandez April 12, 2024 11:58
Copy link
Member

@francoisfreitag francoisfreitag left a comment

Choose a reason for hiding this comment

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

Je ne me suis pas trop plongé dans le code. En survolant et vérifiant quelques vues au hasard, ça m’a l’air bien.

def is_list(url):
url_info = urlsplit(url)
url_path_last_part = url_info.path.split("/")[-1]
return any(url_path_last_part.endswith(postfix) for postfix in ["list", "results"])
Copy link
Member

Choose a reason for hiding this comment

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

Humpf, c’est assez crado. Après, si ça casse, on n’affichera juste pas “à la liste”, et si on doit maintenir une métadonnée qu’une vue est une ListView, y compris pour les function-based views, il y aura aussi des oublis. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C'est pour ça que j'ai ajouté des assert_previous_step dans tout plein de tests pour m'assurer que c'est en place.
Mais oui, c'est un peu crado en attendant d'avoir une idée de génie

Copy link
Contributor

Choose a reason for hiding this comment

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

Je confirme la craditude mais return url_path_last_part.endswith(("list", "results")) permet d'être crado de manière plus courte.

@tonial tonial force-pushed the alaurent/breadcrumb_is_dead branch 3 times, most recently from 106cb72 to fa828f5 Compare April 22, 2024 05:28
@tonial tonial force-pushed the alaurent/breadcrumb_is_dead branch 2 times, most recently from 604effb to 16d8001 Compare April 29, 2024 11:00
@tonial tonial added the modifié Modifié dans le changelog. label Apr 29, 2024
@tonial tonial force-pushed the alaurent/breadcrumb_is_dead branch from 16d8001 to 2b6c201 Compare April 29, 2024 11:40
@tonial
Copy link
Contributor Author

tonial commented Apr 29, 2024

Pour gérer des parcours de navigation un peu complexes entre les pages d'entreprise et de postes avec un retour à la liste au bon endroit j'ai revu un peu ma gestions de l'url de retour sur ces 2 vues (voir dernier commit de la PR)

image

@francoisfreitag as-tu un avis là dessus ?

@tonial tonial requested a review from francoisfreitag April 29, 2024 11:54
@francoisfreitag
Copy link
Member

C’est l’enfer de devoir garder l’historique de navigation de l’utilisateur de notre côté.

Je me demande si on ne devrait pas limiter les back_url à la seule page de recherche (liste de résultats employeur et fiche de poste).

Dans la fiche de poste ou la fiche entreprise, on garde le back_url de la recherche. Ça évite les boucles, permet de revenir directement à la liste des résultats.
La navigation employeur / fiche de poste n’a pas vraiment d’intérêt à être historisée, puisque chacun est accessible depuis la page de l’autre.

Qu’en penses-tu ?

(j’espère que c’est clair)

@tonial tonial force-pushed the alaurent/breadcrumb_is_dead branch 2 times, most recently from 3767fa1 to 729fb8a Compare April 29, 2024 15:13
Copy link
Contributor

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

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

Pas trop fan de tous ces back_url qu'on va se trimbaler et j'étais plutôt dans l'équipe du fil d'ariane mais bon 🤷‍♂️

def is_list(url):
url_info = urlsplit(url)
url_path_last_part = url_info.path.split("/")[-1]
return any(url_path_last_part.endswith(postfix) for postfix in ["list", "results"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Je confirme la craditude mais return url_path_last_part.endswith(("list", "results")) permet d'être crado de manière plus courte.

@tonial
Copy link
Contributor Author

tonial commented Apr 30, 2024

Je préfère aussi le fils d'ariane, mais dans tous les cas, on aurait eu une construction complexe à cause des différents points d'entrée possible des pages...

@tonial tonial force-pushed the alaurent/breadcrumb_is_dead branch from 729fb8a to d9b5cd7 Compare April 30, 2024 08:42
@tonial
Copy link
Contributor Author

tonial commented Apr 30, 2024

Niveau passage des back_url :
On a déjà un soucis d'avoir un certain nombre de chemins possibles pour arriver à une page donnée, donc soit on fait comme ça, soit on utilise un middleware pour stocker le chemin de navigation (et peut être gérer proprement le fait que ce soit une liste ou non au moment d'insérer le lien dans la liste)
Sauf que ça va probablement aussi demander de mettre à zéro en passant par certaines pages, ça ne fonctionnera plus en faisant un retour arrière du navigateur.
Donc je ne suis pas certain que la solution middleware fonctionne bien.

Copy link
Member

@francoisfreitag francoisfreitag left a comment

Choose a reason for hiding this comment

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

Merci pour les pointeurs 😊

@tonial tonial force-pushed the alaurent/breadcrumb_is_dead branch from d9b5cd7 to 63a0784 Compare April 30, 2024 12:32
@tonial tonial enabled auto-merge April 30, 2024 12:33
@tonial tonial added this pull request to the merge queue Apr 30, 2024
Merged via the queue into master with commit c237ca8 Apr 30, 2024
@tonial tonial deleted the alaurent/breadcrumb_is_dead branch April 30, 2024 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC modifié Modifié dans le changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants