magic-hat, add multiple datasets
AbandonedPublic

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

Diff Detail

Repository
R2 GCompris
Branch
gsoc_akshaychd_magicianhat_multiple_datasets
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13321
Build 13339: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
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–67

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–68

missing spaces between operators and operands.

87

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

131

Why not use a nested loop from 0 to 2?

143

nested loop would be better

155–162

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

164–171

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
29

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
27

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 ↗(On Diff #60680)

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

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

168

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
jjazeix commandeered this revision.Jan 4 2020, 8:38 PM
jjazeix abandoned this revision.
jjazeix edited reviewers, added: AkshayCHD; removed: jjazeix.