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

Computenextstep learner #984

Merged
merged 5 commits into from
Sep 28, 2017
Merged

Computenextstep learner #984

merged 5 commits into from
Sep 28, 2017

Conversation

jfmengels
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Sep 25, 2017

Codecov Report

Merging #984 into learner will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           learner     #984      +/-   ##
===========================================
+ Coverage    92.79%   92.87%   +0.08%     
===========================================
  Files          236      235       -1     
  Lines         3149     3185      +36     
===========================================
+ Hits          2922     2958      +36     
  Misses         227      227
Impacted Files Coverage Δ
...oorpacademy-progression-engine/src/config/index.js 100% <ø> (ø) ⬆️
...ademy-app-player/src/view/state-to-props/player.js 100% <ø> (ø) ⬆️
...ademy-progression-engine/src/create-progression.js 100% <ø> (ø) ⬆️
...rpacademy-app-player/src/reducers/data/contents.js 100% <100%> (ø) ⬆️
...coorpacademy-app-player/src/utils/state-extract.js 100% <100%> (ø) ⬆️
...oorpacademy-app-player/src/actions/api/contents.js 100% <100%> (ø) ⬆️
...emy-progression-engine/src/config/microlearning.js 100% <100%> (ø) ⬆️
...ponents/src/molecule/slides/slides-player/index.js 98.85% <100%> (+0.07%) ⬆️
...cademy-progression-engine/src/compute-next-step.js 100% <100%> (ø) ⬆️
...progression-engine/src/check-answer-correctness.js 100% <100%> (ø) ⬆️
... and 5 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 0d5fdb6...64089ba. Read the comment docs.

@@ -7,8 +7,7 @@ export type ViewedResource = {
};

export type Step = {
current: number,
total: number
current: number
Copy link
Contributor

Choose a reason for hiding this comment

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

on se fait un currentStep:int non ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Non, car ça demanderait de repasser sur toutes les progressions actuelles (vu que c'est dans le state initial) :/

Copy link
Contributor

Choose a reason for hiding this comment

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

y aura du menage pour les engine version 2 : )

@chrisdugne chrisdugne mentioned this pull request Sep 27, 2017
@chrisdugne chrisdugne force-pushed the computenextstep-learner branch from f45d4d7 to e2fb2c9 Compare September 27, 2017 15:41
import head from 'lodash/fp/head';
import pipe from 'lodash/fp/pipe';
import unset from 'lodash/fp/unset';
import {pipe, head, get, unset} from 'lodash/fp';
Copy link
Contributor

Choose a reason for hiding this comment

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

Pour le tree-shaking, utilise require('lodash/fp/pipe') & co


const getSlidePool = (
config: Config,
slidePools: Array<{chapterId: string, slides: Array<Slide>}>,
Copy link
Contributor

Choose a reason for hiding this comment

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

ça pourrait être une Map et non un Array.

Copy link
Contributor

Choose a reason for hiding this comment

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

En fait non. On préfère s'assurer de l'ordre plutôt qu'avoir une contrainte d'unicité sur le chapterId.
👍

Copy link
Contributor

Choose a reason for hiding this comment

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

jai eu la meme remarque, @jfmengels m'a fait la meme reponse ;)

@chrisdugne chrisdugne force-pushed the computenextstep-learner branch from 40fc803 to 64089ba Compare September 28, 2017 09:50
@chrisdugne chrisdugne merged commit 0ad77d4 into learner Sep 28, 2017
@chrisdugne chrisdugne deleted the computenextstep-learner branch September 28, 2017 09:53
@@ -50,6 +50,8 @@ export const getCurrentContent = state => {
return getContent(type, ref)(state);
};

export const getContentInfo = pipe(getCurrentContent, get('info'));
export const getNbSlides = pipe(getContentInfo, get('nbSlides'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

export const getNbSlides = pipe(getContentInfo, get('nbSlides'));
->
export const getNbSlides = pipe(getCurrentContent, get('info.'nbSlides'));

Tu peux virer getContentInfo vu que tu l'utilises pas. Si on rajoute de nouvelles informations plus tard, on pourra voir ça à ce moment.

import chaptersData from './chapters.data';
import levelsData from './levels.data';
import {findById} from './slides';

const toMap = reduce((map, object) => map.set(object._id, object), new Map());
const toMap = reduce((map, object) => map.set(object._id, object));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tu aurais aussi pu faire

const toMap = data => reduce((map, object) => map.set(object._id, object), new Map(), data);

Ce permet d'éviter de changer chapters et levels

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.

3 participants