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

Première passe pour la génération dynamique de components #578

Merged
merged 21 commits into from Mar 15, 2019

Conversation

UnbearableBear
Copy link
Contributor

Vous en pensez quoi ??

@codecov
Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #578 into manu/parseXMLtoJSON will increase coverage by 1.02%.
The diff coverage is 83.68%.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           manu/parseXMLtoJSON     #578      +/-   ##
=======================================================
+ Coverage                73.68%   74.71%   +1.02%     
=======================================================
  Files                      122      136      +14     
  Lines                     1239     1380     +141     
  Branches                   189      204      +15     
=======================================================
+ Hits                       913     1031     +118     
- Misses                     288      309      +21     
- Partials                    38       40       +2
Flag Coverage Δ
#true 73.88% <88.54%> (+1.73%) ⬆️
Impacted Files Coverage Δ
packages/code-du-travail-ui/src/Article.js 77.77% <ø> (ø) ⬆️
...rontend/src/FicheServicePublic/components/Table.js 0% <0%> (ø)
packages/code-du-travail-ui/src/keyframes.js 0% <0%> (ø)
packages/code-du-travail-ui/src/theme.js 0% <0%> (ø)
packages/code-du-travail-ui/src/VerticalArrow.js 100% <100%> (ø)
...d/src/FicheServicePublic/components/OuSAdresser.js 100% <100%> (ø)
...end/src/FicheServicePublic/components/Accordion.js 100% <100%> (ø)
...rontend/src/FicheServicePublic/components/Title.js 100% <100%> (ø)
...d/src/FicheServicePublic/components/LienExterne.js 100% <100%> (ø)
...rc/FicheServicePublic/components/ServiceEnLigne.js 100% <100%> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f01d55...e04b404. Read the comment docs.

@lionelB
Copy link
Contributor

lionelB commented Mar 6, 2019

un lien vers les definitions qui permet de mieux comprendre comment les contenu sont organisés https://www.data.gouv.fr/fr/datasets/r/d1b0f744-c997-48d2-9ec4-1c64e82202d6

@lionelB lionelB changed the base branch from master to manu/parseXMLtoJSON March 7, 2019 10:13
@UnbearableBear UnbearableBear reopened this Mar 7, 2019
@revolunet
Copy link
Member

😍 Clean code 👍

Je n'ai juste pas compris ce qu'était SrOnly

Pour les variables CSS, ce serait compliqué de les déplacer dans le package css ?

https://www.npmjs.com/package/postcss-external-vars

@revolunet
Copy link
Member

tout ça :)

capture d ecran 2019-03-07 a 20 47 45

@revolunet
Copy link
Member

revolunet commented Mar 7, 2019

Ce serait pas plus "idiomatique" d'avoir un composant <FicheServicePublic/> qu'une fonction ficheBuilder ?

@UnbearableBear
Copy link
Contributor Author

Yes j'ai hésité un petit bout de temps et comme c'était récursif je me suis dit qu'un component récursif c'est pas commun donc je vais juste faire une fonction mais, effectivement, ça fait bizarre au final

@UnbearableBear
Copy link
Contributor Author

😍 Clean code 👍

Je n'ai juste pas compris ce qu'était SrOnly

Pour les variables CSS, ce serait compliqué de les déplacer dans le package css ?

https://www.npmjs.com/package/postcss-external-vars

Yes mais je le fais dans une PR à part juste après alors ?

Copy link
Contributor

@lionelB lionelB left a comment

Choose a reason for hiding this comment

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

désolé pour le retard, juste des petites remarques

Copy link
Contributor

@lionelB lionelB left a comment

Choose a reason for hiding this comment

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

Désolé pour la revue tardive, pas de grosses modif.

case "Tableau":
return <Table data={data} headingLevel={headingLevel} />;
case "Texte":
if (data.$.find(child => child.name === "Chapitre")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Est ce qu'on pourrait pas tester le niveau via headingLevel ?
Les accordéon de chapitre ne sont en général présent qu'au sous niveau 1

@@ -0,0 +1,21 @@
// beware, this one is recursive
Copy link
Contributor

Choose a reason for hiding this comment

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

Est ce qu'on pourrait pas bouger le fichier au niveau au dessus et l'appeller utils.js

Copy link
Contributor

@lionelB lionelB left a comment

Choose a reason for hiding this comment

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

Nickel les tests 👍
juste 2 trois remarques, et les 2 premiers commit de la branche sont a virer (on peut le faire ensemble)

@lionelB lionelB merged commit 2d24d00 into manu/parseXMLtoJSON Mar 15, 2019
@lionelB lionelB deleted the manu/jsontocomponents branch March 15, 2019 10:10
UnbearableBear added a commit that referenced this pull request Mar 15, 2019
* feat: still working on json format of xml

* feat: working on json format of xml

* feat: finish json format of xml

* refactor: remove pretty option on json file

* fix: useless node_modules in gitignore

* fix: wrong format in raw data, object instead of string

* fix: remove useless python file and optimise json output

* fix: wrong indent

* fix: wrong regexp causing useless fiches to be indexed

* fix: update wrong command line

* fix: remove legacy file

* feat: update format file

* feat: update format file

* feat: update fiches sp according to new filter

* refactor: filters

* refactor: change concat to be more readable

* refactor: remove useless key in dataset

* refactor: apply feedbacks

* Première passe pour la génération dynamique de components (#578)

* feat: first step in generating recursively react components from json

* feat: create new components and add css vars

* feat: add some new components

* feat: add tabulator component

* feat: add more components

* refactor: change ficheBuilder into a component

* feat: minor css changes

* feat: add two more easy components

* refactor: apply pr feedback

* feat: add comment on special case

* feat: add minimal test

* test: update snapshot

* test: adding more tests

* test: add a few more tests

* test: update snapshot

* tests: remplace js mock file to json mock file

* refactor: remove weird heading level

* refactor: migrate css folder into ui

* tests: add missing test

* test: add missing snapshots

* test: update snapshots
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.

None yet

3 participants