-
Notifications
You must be signed in to change notification settings - Fork 1
module page #92
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
base: master
Are you sure you want to change the base?
module page #92
Conversation
NAN-21 Page de module
Kouamé Rameaux Koffi 2021-10-13 pull request : #80 Kouamé Rameaux Koffi 2021-10-08 pull request : #80 |
Deploying with
|
| Latest commit: |
6e69893
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0b8d1eba.platform-nan-dev-8sl.pages.dev |
app.jsx
Outdated
| import { Router } from './lib/router.js' | ||
| import { Profile } from './page/profile.jsx' | ||
| import { Home } from './page/home.jsx' | ||
| import {Module} from './page/module.jsx' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
il manque prettier la
component/stackLi.jsx
Outdated
| export const StackLi = ({ data: { type, name } }) => { | ||
| return ( | ||
| <li class="stackli"> | ||
| <Span class={`${type === 'exercise' ? 'square' : 'circle'}`} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pourqoui creer une string c'est deja des strings, fait plutot:
<Span class={type === 'exercise' ? 'square' : 'circle'} />
page/module.jsx
Outdated
| { name: '06-Duplicate', type: 'exercise' }, | ||
| { name: '07-cool', type: 'quiz' }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alors je suis desoler mais une partie de mes remarques n'avais pas ete publier:
#80 (review)
si tu peu recheck, notament niveau de la structure
kigiri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il faut que tu formatte ta donner pour qu'elle soit adapter a ton utilisation, la tu la manipule de partout c'est pas optimal
data/fakeModule.json
Outdated
| { | ||
| "name": "02-Anything To Declare", | ||
| "type": "exercise", | ||
| "key": "02-Anything To Declare" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bon c'est pas important pour ce code la, mais en toute logique la key correspondra au nom du fichier, alors que le name lui sera le titre (H1) du markdown du sujet
page/module.jsx
Outdated
| data={rest} | ||
| isPassed={isPassed(rest.name)} | ||
| isNext={isNext(rest.name)} | ||
| infos={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
infos est aussi imprecis que data comme nom, a eviter le plus possible.
page/module.jsx
Outdated
| {fakeModule.children.map(({ key, ...rest }) => ( | ||
| <StackLi | ||
| key={key} | ||
| data={rest} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data... j'ai pas deja fait une remarque sur ce genre de nom de proprieter / variable ?
ca dit rien ! c'est quoi comme data ?
pourquoi ne pas juste spread les props:
fakeModule.children.map((child) => (
<StackLi {...child} result={userInfo.results[child.key]} />
))
page/module.jsx
Outdated
| <ul class="stack"> | ||
| {fakeModule.configs.display.map((s) => ( | ||
| <StackLi key={s.name} data={s} /> | ||
| {fakeModule.children.map(({ key, ...rest }) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
je comprend pas pourquoi t'a besoin du isNext, et pas sur du nom de la variable tu veut dire que c'est le prochain qui est possible a faire ? ou la tache en cours ?
Dans ce cas ca serais plus current ou isActive
page/module.jsx
Outdated
| const isNext = (name) => { | ||
| const [module, step] = userInfo.level | ||
| .split('-') | ||
| .map((r) => parseInt(r.slice(1))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C'est mieu d'utiliser Number que parseInt
page/module.jsx
Outdated
| userProgress.find((r) => getExoNumber(r[0]) === exoNumber)[1].pass) | ||
| ) { | ||
| return true | ||
| } else return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ce genre de code:
if (
module > moduleNumber ||
(module === moduleNumber &&
step >= exoNumber &&
userProgress.find((r) => getExoNumber(r[0]) === exoNumber)[1].pass)
) {
return true
} else return falseCa va pas, ta condition est trop complexe, decompose la:
// est-ce qu'on devrais ce fier a ca ?
if (module < moduleNumber) return true
// trying to peek ?
if (module > moduleNumber || step < exoNumber) return false
return userProgress.find((r) => getExoNumber(r[0]) === exoNumber)[1]?.passMais toute cette fonction ne sert a rien
| "name": "05-Redeclaration Love", | ||
| "name": "Redeclaration Love", | ||
| "type": "exercise", | ||
| "key": "05-Redeclaration of Love" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pas tres important mais les keys seront pas genre: 05-Redeclaration of Love mais le nom du fichier donc plus un truc genre: 05-redeclaration-of-love.exercise.md je pense
page/module.jsx
Outdated
| : null | ||
| } | ||
| {...child} | ||
| name={child.key} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pourquoi name={child.key} ? alors que le child a deja un name ?
| const iconType = { | ||
| quiz: '📚', | ||
| exercise: '🖊', | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
le donne statique comme ca vaut mieu les sortir que les redeclarer a chaques fois, ca changes pas grand chose mais c'est toujours preferable
component/stackLi.jsx
Outdated
| } | ||
| .module .stackli span.name { | ||
| font-size: 1.2rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
j'ai fait un commentaire sur changer la font-size et les margin, l'idee c'est que l'affichage corresponde a ce qu'un editeur de texte peu generer.
148be57 to
ebf26da
Compare
component/stackLi.jsx
Outdated
| {result && <> <Div class="second_block"> | ||
| {' '} | ||
| (<Span class="attempts">{result.attempts}-attempts</Span> | ||
| {result.seconds && <> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<> 😨
t'a passer prettier la dessus ?
| children, | ||
| ), | ||
| ]), | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool mais gere l'espacement avec un padding ou une marge plutot que  , tu peu indiquer 1ch pour avoir la bonne valeur
make some module page