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

feat(chart): SKFP-692 add pie and chart graphics #281

Merged
merged 1 commit into from
May 18, 2023

Conversation

lflangis
Copy link
Contributor

@lflangis lflangis commented May 17, 2023

FEATURE

Description

Afin d'unifier nos projets, les graphiques seront maintenant géré côté ferlab-ui.

Pour le moment, nivo reste en 0.80 (et non en 0.83 à cause de ce ticket). Sauf que la verison 0.83 contenait des fix pour les tests unitaires.

Acceptance Criterias

Validation de Qualité

  • Validation du design avec design figma (dev)
  • QA - Validation des critère de succès (via screenshot)
  • Design/UI - Validation du respect du design/theme

Screenshot

image

Mention

@luclemo @kstonge

@github-actions
Copy link

github-actions bot commented May 17, 2023

Coverage report for packages/ui

St.
Category Percentage Covered / Total
🟢 Statements 100% 0/0
🟢 Branches 100% 0/0
🟢 Functions 100% 0/0
🟢 Lines 100% 0/0

Test suite run success

0 tests passing in 0 suite.

Report generated by 🧪jest coverage report action from 75ddb61

@github-actions
Copy link

github-actions bot commented May 17, 2023

Project Coverage and Test

Statements : 29.18% ( 3375/11564 )
Branches : 54.15% ( 241/445 )
Functions : 25.93% ( 76/293 )
Lines : 29.18% ( 3375/11564 )

Test Suites: 20 passed, 20 total
Tests: 91 passed, 91 total
Snapshots: 0 total
Time: 82.701 s
Ran all test suites.

@kstonge
Copy link
Member

kstonge commented May 17, 2023

J'sais pas si c'est dans le scope de cette PR, mais possible de centrer en hauteur les éléments de la carte Demographics? Mettre le même espacement en haut du titre des graphiques qu'en bas des graphiques.

@lflangis
Copy link
Contributor Author

@kstonge on peut l'ajuster côté client, il suffit de jouer avec la props margin

image

@luclemo
Copy link
Member

luclemo commented May 17, 2023

Puisqu'on parle de l'affichage, je dirais que les titres sont trop gros. De plus, je les avait mis en bas pour de raison d'alignement, ex: au cas où il y aurait une carte à coté avec des graphiques sans titre. Pour ce dernier point je suis ouvert à en discuter si ça pose problème.
image

@luclemo
Copy link
Member

luclemo commented May 17, 2023

Même commentaire que Karine— ci c'est out-of-scope ici, laisse faire on y reviendra là où il faut ✌️

@lflangis
Copy link
Contributor Author

lflangis commented May 17, 2023

Le tite est mon oublie @luclemo je vais faire un fix.

@lflangis
Copy link
Contributor Author

lflangis commented May 17, 2023

@kstonge @luclemo

Ça devrait être mieux, j'ai du tricher un peu car j'ai appliqué un woraround dans Nivo afin que les composants responsives fonctionnent correctement.

image

@lflangis lflangis force-pushed the feat/SKFP-692/responsive-pie branch from eba0812 to 5a77e63 Compare May 17, 2023 19:21
@luclemo
Copy link
Member

luclemo commented May 17, 2023

@lflangis Si c'est le theme de KF, les titres sont toujours mauve (geekblue-7) comme le titre de la carte. Voir maquette KF:
https://www.figma.com/file/WP195zO40mzIX9j6I2AfzJ/KF---Summary-View-2.0?type=design&node-id=609-46957&t=yu70tHGNnlKla9DW-4
Pasted_Image_2023-05-17__3_25_PM

@lflangis
Copy link
Contributor Author

@luclemo en soit c'est fait, j'ai simplement poster une screenshot sans surcharge du theme car cette tâche est ferlab et ne devrait pas concerner le theme de KF (une PR KF sera créé).

Mais voici une screenshot avec le theme surchargé, j'aurais en effet pu l'oublier 😅
image

@lflangis lflangis requested a review from claudia1296 May 18, 2023 12:12
Copy link
Contributor

@atoulous atoulous left a comment

Choose a reason for hiding this comment

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

lgtm

packages/ui/src/layout/ResizableGridLayout/index.tsx Outdated Show resolved Hide resolved
@lflangis lflangis force-pushed the feat/SKFP-692/responsive-pie branch from 5a77e63 to 75ddb61 Compare May 18, 2023 14:34
@lflangis lflangis merged commit 137f076 into master May 18, 2023
@lflangis lflangis deleted the feat/SKFP-692/responsive-pie branch May 18, 2023 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants