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

1355 tunnel de conversion de bout en bout juin 2023 #95

Merged
merged 3 commits into from
Aug 17, 2023

Conversation

baptou12
Copy link
Contributor

@baptou12 baptou12 commented Jul 20, 2023

Commencement de la tâche, le front est quasi fait, il manque d'aller chercher la vrai donnée. A priori je vais mettre ça dans aides-jeunes car la connexion à la db pour certaines métriques se trouve là bas.

Preneur de retours UI/UX

Edit: en desktop les chart sont désormais aligné horizontalement :
image

Warning partiellement obsolète car les nouvelles stats ont été mergé sur le master d'aides-jeunes
⚠️ Pour tester en full local c'est pas évident, mais ça nous montre des lacunes sur l'environnement de dev, il faut :

  app.use(cors());
  app.use(express.static(path.join(__dirname, "../dist")))

(importer cors et path en plus)

  • Changer l'url aidesJeunesStatisticsURL dans le fichier next.config.js de mes-aides-analytics

@baptou12 baptou12 force-pushed the 1355-tunnel-de-conversion-de-bout-en-bout-juin-2023 branch 2 times, most recently from f590b40 to e6020f9 Compare July 20, 2023 14:31
services/funnelService.js Outdated Show resolved Hide resolved
@baptou12 baptou12 force-pushed the 1355-tunnel-de-conversion-de-bout-en-bout-juin-2023 branch 6 times, most recently from 81287b9 to 2574dfa Compare July 25, 2023 09:58
@baptou12 baptou12 marked this pull request as ready for review July 25, 2023 10:15
@baptou12 baptou12 requested a review from a team July 25, 2023 10:15
@baptou12 baptou12 force-pushed the 1355-tunnel-de-conversion-de-bout-en-bout-juin-2023 branch from 095ad4f to 1f52a3d Compare July 25, 2023 10:18
Comment on lines +35 to +39
const statsResponse = await Fetch.getJSON(
configuration.env.aidesJeunesStatisticsURL,
)

const funnelData = statsResponse.funnel
Copy link
Contributor

Choose a reason for hiding this comment

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

Pour tester, j'ai remplacé le fetch ici par un objet en dur car actuellement l'objet renvoyé par https://mes-aides.1jeune1solution.beta.gouv.fr/documents/stats.json n'a pas encore la clef funnel. Tu as testé avec un lien en pré-prod ou objet hébergé ailleurs j'imagine ?

pages/funnel.js Outdated
Comment on lines 7 to 11
constructor(props) {
super(props)

this.state = {
chartsData: null,
loading: true,
selectedMonth: null,
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

On peut se passer de constructeur ici

Suggested change
constructor(props) {
super(props)
this.state = {
chartsData: null,
loading: true,
selectedMonth: null,
}
}
state = {
chartsData: null,
loading: true,
selectedMonth: null,
}

pages/funnel.js Outdated
Comment on lines 34 to 43
if (this.state.loading) {
return <p>Chargement...</p>
}

const currentMonthData = this.state.chartsData[this.state.selectedMonth]

return (
<>
<h1 data-testid="title">
Metriques de parcours {this.state.selectedMonth}
Copy link
Contributor

@Shamzic Shamzic Jul 27, 2023

Choose a reason for hiding this comment

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

Utilisation de déstructuration possible + coquille "Métriques"

Suggested change
if (this.state.loading) {
return <p>Chargement...</p>
}
const currentMonthData = this.state.chartsData[this.state.selectedMonth]
return (
<>
<h1 data-testid="title">
Metriques de parcours {this.state.selectedMonth}
const { loading, selectedMonth, chartsData } = this.state
if (loading) {
return <p>Chargement...</p>
}
const currentMonthData = chartsData[selectedMonth]
return (
<>
<h1 data-testid="title">
Métriques de parcours {selectedMonth}

@baptou12 baptou12 force-pushed the 1355-tunnel-de-conversion-de-bout-en-bout-juin-2023 branch 2 times, most recently from 33ca1e5 to 51b21c6 Compare August 17, 2023 08:50
Comment on lines 27 to 34
margin={{ top: 15, right: 10, bottom: 50, left: 60 }}
padding={0.1}
animate={false}
colors={["#a4c4eb", "#d62728", "#ff7f0e"]}
axisBottom={{
tickSize: 5,
tickPadding: 5,
tickRotation: -15,
Copy link
Contributor

Choose a reason for hiding this comment

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

J'aurais tendance à mettre tickRotation à -45 pour que ça soit cohérent avec les autres pages en augmentant la propriété bottom de margin à 150 pour pas que le texte soit coupé :
image

@baptou12 baptou12 force-pushed the 1355-tunnel-de-conversion-de-bout-en-bout-juin-2023 branch from 51b21c6 to 423e23c Compare August 17, 2023 13:10
Comment on lines 400 to 418
.funnel-charts {
display: flex;
flex-direction: row;
}

.funnel-charts .funnel-chart {
flex: 1;
}

.funnel-charts .funnel-chart h2 {
min-height: 4rem;
}

@media screen and (max-width: 1024px) {
.funnel-charts {
flex-direction: column;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

J'ai un bug un peu mystique avec l'affichage de la page du tunnel de conversion qui est dû je pense à la lib qui gère les graphes et leur redimensionnement :

  • aller sur la page du tunnel
  • redimensionner la page jusqu'au passage en colonne des graphiques (1024px de large)
  • re-redimensionner la page à sa largeur d'origine
  • les graphes ont gardé leur largeur de 100% et débordent maintenant de la page

Je n'ai plus le bug avec le code suivant :

.funnel-charts {
  display: flex;
  flex-direction: row;
  flex-wrap: wrap;
}

.funnel-charts .funnel-chart {
  flex: 1;
  max-width: calc(100% / 3);
}

.funnel-charts .funnel-chart h2 {
  min-height: 4rem;
}

@media screen and (max-width: 1024px) {
  .funnel-charts {
    flex-direction: column;
  }
  .funnel-charts .funnel-chart {
    max-width: 100%;
  }
}
  • flex-wrap permet de gérer le retour à la ligne des graphes
  • max-width permet de faire que les graphes font toujours leur taille maximale autorisée (en plus du flex: 1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok même comportement, je pense que la lib souhaite qu'on explicite les largeur un peu plus pour gérer le redimensionnement. J'ai repris ta solution et j'ai effectivement plus le soucis ! Merci !

public/static/style.css Outdated Show resolved Hide resolved
@baptou12 baptou12 force-pushed the 1355-tunnel-de-conversion-de-bout-en-bout-juin-2023 branch from 423e23c to f66bd35 Compare August 17, 2023 14:05
Copy link
Contributor

@Cugniere Cugniere left a comment

Choose a reason for hiding this comment

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

Ok pour moi, rien à redire de plus

@baptou12 baptou12 dismissed Shamzic’s stale review August 17, 2023 14:36

Simon, j'ai fait tes retours, je dismiss pour cause de vacances :-)

@baptou12 baptou12 merged commit cbdd1a9 into main Aug 17, 2023
3 checks passed
@guillett guillett added this to the BC 1406243900 milestone Oct 17, 2023
@guillett guillett deleted the 1355-tunnel-de-conversion-de-bout-en-bout-juin-2023 branch February 7, 2024 09:59
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