magic-hat, add multiple datasets
Needs ReviewPublic

Authored by AkshayCHD on Jun 19 2019, 6:33 AM.

Diff Detail

Repository
R2 GCompris
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
timotheegiet requested changes to this revision.Jun 21 2019, 2:23 PM
timotheegiet added a subscriber: timotheegiet.
timotheegiet added inline comments.
src/activities/magic-hat-minus/magic-hat.js
36–37 ↗(On Diff #60218)

Please improve the naming of those var.

This revision now requires changes to proceed.Jun 21 2019, 2:23 PM

lots of trailing whitespaces.

src/activities/magic-hat-minus/MagicHat.qml
68

"calculation_range" would be better?

src/activities/magic-hat-minus/magic-hat.js
66 ↗(On Diff #60218)

don't use hardcoded values. Keep it in a variable and use that variable it. Will be more flexible and easy for anyone to find modify it he wants.
Also, please add a comment mentioning why items.range / 30 <= 1 is done. Improves readability of the code.

67 ↗(On Diff #60218)

missing spaces between operators and operands.

86 ↗(On Diff #60218)

Put the expressions in parenthesis. Same comment for the ones below.
Improves code readability.

130 ↗(On Diff #60218)

Why not use a nested loop from 0 to 2?

142 ↗(On Diff #60218)

nested loop would be better

157 ↗(On Diff #60218)

The same thing for this formula like I said previously for the one above.

166 ↗(On Diff #60218)

Can you rename these two variables to more understandable names (this one and the one below)?

amankumargupta requested changes to this revision.Jun 22 2019, 5:04 PM
jjazeix added inline comments.
src/activities/magic-hat-minus/StarsBar.qml
48

use .arg() for translation, on some locale, it may not be "3 x" but "3x" or even "x3" or not x at all.
There are also 2 spaces, I think it's 0 in English.

src/activities/magic-hat-plus/resource/4/Data.qml
28

if it's always an int, use int as type. Note that a range is usually two bounds, not a single number, it's more a maxValue here

AkshayCHD updated this revision to Diff 60573.Jun 24 2019, 12:34 PM
  • magic-hat, improve variable names and fix typos
AkshayCHD marked 11 inline comments as done.Jun 24 2019, 12:39 PM

@timotheegiet @amankumargupta @jjazeix I have updated the diff, you may have a look :)

This revision was not accepted when it landed; it landed in state Needs Review.Jun 24 2019, 12:43 PM
This revision was automatically updated to reflect the committed changes.
AkshayCHD reopened this revision.Jun 24 2019, 12:45 PM
AkshayCHD edited the test plan for this revision. (Show Details)
AkshayCHD updated this revision to Diff 60575.Jun 24 2019, 12:49 PM
  • magic-hat, add multiple datasets
  • magic-hat, improve variable names and fix typos
AkshayCHD edited the summary of this revision. (Show Details)Jun 24 2019, 12:50 PM
AkshayCHD updated this revision to Diff 60576.Jun 24 2019, 12:57 PM
  • magic-hat-plus, update ActivityInfo.qml to include newly added levels
This revision was not accepted when it landed; it landed in state Needs Review.Jun 24 2019, 12:58 PM
This revision was automatically updated to reflect the committed changes.
AkshayCHD reopened this revision.Jun 24 2019, 12:59 PM
AkshayCHD updated this revision to Diff 60578.Jun 24 2019, 1:00 PM
  • magic-hat, add multiple datasets
  • magic-hat, improve variable names and fix typos
  • magic-hat-plus, update ActivityInfo.qml to include newly added levels
jjazeix added inline comments.Jun 24 2019, 5:27 PM
src/activities/magic-hat-minus/MagicHat.qml
68

int?

src/activities/magic-hat-minus/StarsBar.qml
35

int?

48

("%1x").arg(...)
with a comment for translators above please :) (//: comment)

src/activities/magic-hat-minus/resource/5/Data.qml
26

up to

AkshayCHD updated this revision to Diff 60677.Jun 26 2019, 9:37 AM
  • magic-hat, add comments and fix typos
This revision was not accepted when it landed; it landed in state Needs Review.Jun 26 2019, 9:40 AM
This revision was automatically updated to reflect the committed changes.
AkshayCHD marked an inline comment as done.
AkshayCHD reopened this revision.Jun 26 2019, 9:46 AM
AkshayCHD updated this revision to Diff 60680.Jun 26 2019, 9:46 AM
  • magic-hat, add multiple datasets
  • magic-hat, improve variable names and fix typos
  • magic-hat-plus, update ActivityInfo.qml to include newly added levels
  • magic-hat, add comments and fix typos
  • magic-hat, include an unsaved change
AkshayCHD marked 2 inline comments as done.Jun 26 2019, 9:47 AM
jjazeix added inline comments.Jun 26 2019, 10:44 AM
src/activities/magic-hat-minus/resource/1/Data.qml
31

not needed?

AkshayCHD updated this revision to Diff 60692.Jun 26 2019, 2:39 PM
  • magic-hat, remove unwanted parameters from dataset
AkshayCHD marked an inline comment as done.Jun 26 2019, 2:40 PM
AkshayCHD marked an inline comment as done.Jun 26 2019, 3:06 PM
timotheegiet accepted this revision.Jun 27 2019, 1:58 PM

All the comments have been addressed, looks good for me.

scagarwal accepted this revision.Jun 27 2019, 2:01 PM
scagarwal added a subscriber: scagarwal.

Good for me too.

jjazeix added inline comments.Jun 27 2019, 3:05 PM
src/activities/magic-hat-minus/magic-hat.js
77 ↗(On Diff #60692)

is it needed? If no args, it does nothing?

168 ↗(On Diff #60692)

2 spaces after ==

AkshayCHD updated this revision to Diff 60741.Jun 27 2019, 5:46 PM
  • magic-hat, fix bugs
AkshayCHD marked 2 inline comments as done.Jun 27 2019, 5:46 PM
AkshayCHD updated this revision to Diff 60821.Jun 29 2019, 9:05 AM
  • magic-hat, add activity configuration to change dataset
This revision was not accepted when it landed; it landed in state Needs Review.Jun 29 2019, 9:08 AM
This revision was automatically updated to reflect the committed changes.

The diff is not complete (missing datasets for example). Just one point to fix, on last dataset of minus-hat, it's up to 10000 not 1000.
@echarruau , for higher levels, would it make sense to have an hint on the numbers ? When the coefficients are displayed, and the stars are displayed, it maybe difficult for children to count exactly?

AkshayCHD reopened this revision.Jul 1 2019, 5:27 PM
AkshayCHD updated this revision to Diff 60946.Jul 1 2019, 5:29 PM
  • magic-hat, add multiple datasets
  • magic-hat, improve variable names and fix typos
  • magic-hat-plus, update ActivityInfo.qml to include newly added levels
  • magic-hat, add comments and fix typos
  • magic-hat, include an unsaved change
  • magic-hat, remove unwanted parameters from dataset
  • magic-hat, fix bugs
  • magic-hat, add activity configuration to change dataset
AkshayCHD updated this revision to Diff 60973.Jul 1 2019, 11:11 PM
  • magic-hat, fix typo
This revision was not accepted when it landed; it landed in state Needs Review.Jul 1 2019, 11:11 PM
This revision was automatically updated to reflect the committed changes.
AkshayCHD reopened this revision.Jul 1 2019, 11:11 PM
AkshayCHD updated this revision to Diff 60975.Jul 1 2019, 11:12 PM
  • magic-hat, add multiple datasets
  • magic-hat, improve variable names and fix typos
  • magic-hat-plus, update ActivityInfo.qml to include newly added levels
  • magic-hat, add comments and fix typos
  • magic-hat, include an unsaved change
  • magic-hat, remove unwanted parameters from dataset
  • magic-hat, fix bugs
  • magic-hat, add activity configuration to change dataset
  • magic-hat, fix typo