Added Easy Mode(ref: T5937)
ClosedPublic

Authored by rohitdas on Aug 17 2017, 6:31 PM.

Details

Summary

Please let me know if a button would be better suited visually as a switch. I couldn't find appropriate .svg files for it.

Diff Detail

Repository
R2 GCompris
Lint
Lint Skipped
Unit
Unit Tests Skipped
rohitdas created this revision.Aug 17 2017, 6:31 PM
rohitdas retitled this revision from Added Easy Mode to Added Easy Mode(ref: T5937).Aug 17 2017, 6:34 PM
jjazeix added inline comments.
src/activities/letter-in-word/LetterInWord.qml
246

you should use a GCDialogCheckBox instead of directly a GCText

248

no need to change the text color

249

qsTr needs to be used for translatable texts.
Easy does not help the child, we should find a more explicit text on what the option does

259

it should only be changed when clicking on the cross (so in onSaveData).
You should also check that it is well loaded when you start the activity (onLoadData).
Take a look at categorisation for example to be sure you save and load the good js type.

260

you should not change the text directly

rohitdas updated this revision to Diff 18450.Aug 20 2017, 5:07 PM
rohitdas marked 2 inline comments as done.

Please review changes

jjazeix added inline comments.Aug 20 2017, 7:07 PM
src/activities/letter-in-word/LetterInWord.qml
6

do you need to have both mode and reduceWords?

24

you shouldn't change anything on checking/unchecking the box, it should only be done when the user clicks on the Cross to go back to the activity (onSaveData).

32
60

if I'm not wrong, you never restore the reduceWords when starting the activity

jjazeix added inline comments.Aug 20 2017, 7:13 PM
src/activities/letter-in-word/LetterInWord.qml
67

remove whitespaces

src/activities/letter-in-word/letter-in-word.js
91

remove whitespaces

rohitdas updated this revision to Diff 18507.Aug 21 2017, 5:52 PM
rohitdas marked 3 inline comments as done.

Please look into the changes and let me know.

jjazeix added inline comments.Aug 22 2017, 1:02 PM
src/activities/letter-in-word/LetterInWord.qml
27

still needed?

128

I still don't understand why you change mode here

183

I still don't understand why you use 2 different variables for the same data

src/activities/letter-in-word/letter-in-word.js
93

no need to log

rohitdas updated this revision to Diff 19663.Sep 19 2017, 6:47 AM

Couldn't remove QtQuickControls, else the ExclusiveOptions isn't working. Using a second variable to save the checked state of the dialogCheckBoxes. Let me know if this is what was intended to be done.

jjazeix added a comment.EditedSep 27 2017, 7:06 PM

Please check remarks on Aug 22 2017, you probably don't need 2 variables

src/activities/letter-in-word/LetterInWord.qml
97

missing space

rohitdas updated this revision to Diff 20017.Sep 28 2017, 4:11 AM

Please review.

jjazeix added inline comments.Sep 28 2017, 6:00 AM
src/activities/letter-in-word/LetterInWord.qml
40

why do you change it here? It should only be changed when clicking on the cross (save method). Also, the names of the states are not the good ones.

rohitdas updated this revision to Diff 20073.Sep 29 2017, 4:36 AM

Finally managed to access the .checked property. Please review.

qrc:/gcompris/src/activities/letter-in-word/LetterInWord.qml:163: TypeError: Cannot read property 'locale' of undefined
save configuration for: "letter-in-word"

This is what I get and I didn't see any change with change in config.

src/activities/letter-in-word/LetterInWord.qml
69

why do you keep two variables? You keep mode and two booleans easy normal mode, they both do the same work?

106

the names can be better what is easy mode should be signigicant from the variable name.

qrc:/gcompris/src/activities/letter-in-word/LetterInWord.qml:163: TypeError: Cannot read property 'locale' of undefined
save configuration for: "letter-in-word"

This is what I get and I didn't see any change with change in config.

I am not getting this error. I changed the names to easyModeConfig and normalModeConfig. The mode is to save the mode as a string., the easyMode and normalMode are the id for GCDIalogCheckBoxes.

rohitdas updated this revision to Diff 21437.Oct 27 2017, 2:23 PM

Changed names.

Restricted Application added a project: KDE Edu. · View Herald TranscriptOct 27 2017, 2:23 PM
Restricted Application added a subscriber: KDE Edu. · View Herald Transcript

comments from Timothée:

  • the default option with all words should be first in the list.
  • the options could be named "All words" and "Only 5 words".
src/activities/letter-in-word/LetterInWord.qml
69

why not directly store the number of words as int? It would have more sense than a string "easy", "normal"and would be directly be useable in the js file

rohitdas updated this revision to Diff 21794.Nov 2 2017, 5:36 PM

Changes made as requested. Please review.

jjazeix added inline comments.Nov 2 2017, 7:03 PM
src/activities/letter-in-word/LetterInWord.qml
69

you can rename it to be more precise on what it does
and have "readonly int" variables to name the possible values (reading 5 and 11 at multiple places does not help to know where these values come from and if they are linked or not)

171

"modes" can be renamed to be more precise

rohitdas updated this revision to Diff 22076.Nov 8 2017, 2:05 PM

Changed made as requested. Please review.

jjazeix added inline comments.Nov 17 2017, 3:42 PM
src/activities/letter-in-word/LetterInWord.qml
127

you still use the numbers instead of the variables you defined

155

missing space

rohitdas updated this revision to Diff 22551.Nov 18 2017, 4:18 AM

Changes made as requested. Please review.

jjazeix accepted this revision.Nov 18 2017, 2:14 PM

Thank you,

commited in https://commits.kde.org/gcompris/c48f7057c27166edc6b8297bb3518a10ec2ee31b with changes, please check them.

Johnny

This revision is now accepted and ready to land.Nov 18 2017, 2:14 PM
jjazeix closed this revision.Nov 18 2017, 2:14 PM