Please let me know if a button would be better suited visually as a switch. I couldn't find appropriate .svg files for it.
Details
- Reviewers
jjazeix - Group Reviewers
GCompris GCompris: Improvements - Maniphest Tasks
- T5937: letter_in_word: Easy mode for preschool children
Diff Detail
- Repository
- R2 GCompris
- Lint
Lint Skipped - Unit
Unit Tests Skipped
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. | |
259 | it should only be changed when clicking on the cross (so in onSaveData). | |
260 | you should not change the text directly |
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 | you should use ExclusiveGroup (https://doc.qt.io/qt-5/qml-qtquick-controls-exclusivegroup.html) | |
60 | if I'm not wrong, you never restore the reduceWords when starting the activity |
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.
Please check remarks on Aug 22 2017, you probably don't need 2 variables
src/activities/letter-in-word/LetterInWord.qml | ||
---|---|---|
97 | missing space |
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. |
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. |
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.
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 |
src/activities/letter-in-word/LetterInWord.qml | ||
---|---|---|
69 | you can rename it to be more precise on what it does | |
171 | "modes" can be renamed to be more precise |
Thank you,
commited in https://commits.kde.org/gcompris/c48f7057c27166edc6b8297bb3518a10ec2ee31b with changes, please check them.
Johnny