magic-hat, add multiple datasets
Needs ReviewPublic

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

Diff Detail

Branch
gsoc_akshaychd_multiple_dataset_magician_hat
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12957
Build 12975: arc lint + arc unit
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

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

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

missing spaces between operators and operands.

86

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

130

Why not use a nested loop from 0 to 2?

142

nested loop would be better

157–164

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

166–173

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
49

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?

49

("%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
32

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
76

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

170

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