Changeset View
Changeset View
Standalone View
Standalone View
src/activities/letter-in-word/LetterInWord.qml
| Context not available. | |||||
| 24 | 24 | | |||
|---|---|---|---|---|---|
jjazeix: you shouldn't change anything on checking/unchecking the box, it should only be done when the… | |||||
| 25 | import QtQuick 2.6 | 25 | import QtQuick 2.6 | ||
| 26 | import QtGraphicalEffects 1.0 | 26 | import QtGraphicalEffects 1.0 | ||
| 27 | import QtQuick.Controls 1.5 | ||||
jjazeix: still needed? | |||||
| 27 | import GCompris 1.0 | 28 | import GCompris 1.0 | ||
| 28 | import "../../core" | 29 | import "../../core" | ||
| 29 | import "letter-in-word.js" as Activity | 30 | import "letter-in-word.js" as Activity | ||
you should use ExclusiveGroup (https://doc.qt.io/qt-5/qml-qtquick-controls-exclusivegroup.html) jjazeix: you should use ExclusiveGroup (https://doc.qt.io/qt-5/qml-qtquick-controls-exclusivegroup.html) | |||||
if I'm not wrong, you never restore the reduceWords when starting the activity jjazeix: if I'm not wrong, you never restore the reduceWords when starting the activity | |||||
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. jjazeix: why do you change it here? It should only be changed when clicking on the cross (save method). | |||||
| Context not available. | |||||
| 65 | property alias bar: bar | 66 | property alias bar: bar | ||
| 66 | property alias background: background | 67 | property alias background: background | ||
jjazeix: remove whitespaces | |||||
| 67 | property alias wordsModel: wordsModel | 68 | property alias wordsModel: wordsModel | ||
| 69 | property string mode: "normal" | ||||
why do you keep two variables? You keep mode and two booleans easy normal mode, they both do the same work? dmadaan: why do you keep two variables? You keep mode and two booleans easy normal mode, they both do… | |||||
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 jjazeix: why not directly store the number of words as int? It would have more sense than a string… | |||||
you can rename it to be more precise on what it does jjazeix: you can rename it to be more precise on what it does
and have "readonly int" variables to name… | |||||
| 68 | property GCAudio audioVoices: activity.audioVoices | 70 | property GCAudio audioVoices: activity.audioVoices | ||
| 69 | property alias parser: parser | 71 | property alias parser: parser | ||
| 70 | property alias animateX: animateX | 72 | property alias animateX: animateX | ||
| Context not available. | |||||
| 91 | animateX.restart(); | 93 | animateX.restart(); | ||
| 92 | } | 94 | } | ||
| 93 | 95 | | |||
| 96 | ExclusiveGroup { | ||||
jjazeix: missing space | |||||
| 97 | id: configOptions | ||||
| 98 | } | ||||
| 99 | | ||||
| 94 | DialogActivityConfig { | 100 | DialogActivityConfig { | ||
| 95 | id: dialogActivityConfig | 101 | id: dialogActivityConfig | ||
| 96 | currentActivity: activity | 102 | currentActivity: activity | ||
the names can be better what is easy mode should be signigicant from the variable name. dmadaan: the names can be better what is easy mode should be signigicant from the variable name. | |||||
| Context not available. | |||||
| 107 | Column { | 113 | Column { | ||
| 108 | id: column | 114 | id: column | ||
| 109 | spacing: 10 | 115 | spacing: 10 | ||
| 110 | width: parent.width | 116 | width: dialogActivityConfig.width | ||
| 117 | height: dialogActivityConfig.height | ||||
| 118 | property alias easyMode: easyMode | ||||
| 119 | property alias normalMode: normalMode | ||||
| 120 | | ||||
| 121 | GCDialogCheckBox { | ||||
| 122 | id: easyMode | ||||
| 123 | width: column.width - 50 | ||||
| 124 | text: qsTr("Reduce Visible Words to 5") | ||||
| 125 | checked: (items.mode == "easy") ? true : false | ||||
| 126 | exclusiveGroup: configOptions | ||||
| 127 | onCheckedChanged: { | ||||
jjazeix: you still use the numbers instead of the variables you defined | |||||
| 128 | items.mode = easyMode.checked ? "easyMode" : "normalMode" | ||||
jjazeix: I still don't understand why you change mode here | |||||
| 129 | } | ||||
| 130 | } | ||||
| 131 | | ||||
| 132 | GCDialogCheckBox { | ||||
| 133 | id: normalMode | ||||
| 134 | width: column.width - 50 | ||||
| 135 | text: qsTr("Get All Available Words") | ||||
| 136 | checked: (items.mode == "normal") ? true : false | ||||
| 137 | exclusiveGroup: configOptions | ||||
| 138 | onCheckedChanged: { | ||||
| 139 | items.mode = normalMode.checked ? "normalMode" : "easyMode" | ||||
| 140 | } | ||||
| 141 | } | ||||
| 111 | 142 | | |||
| 112 | Flow { | 143 | Flow { | ||
| 113 | spacing: 5 | 144 | spacing: 5 | ||
jjazeix: missing space | |||||
| Context not available. | |||||
| 125 | 156 | | |||
| 126 | onClose: home() | 157 | onClose: home() | ||
| 127 | onLoadData: { | 158 | onLoadData: { | ||
| 159 | if(dataToSave && dataToSave["mode"]){ | ||||
| 160 | items.mode = dataToSave["mode"] | ||||
| 161 | console.log("Mode: " + items.mode); | ||||
| 162 | } | ||||
| 163 | | ||||
| 128 | if(dataToSave && dataToSave["locale"]) { | 164 | if(dataToSave && dataToSave["locale"]) { | ||
| 129 | background.locale = dataToSave["locale"]; | 165 | background.locale = dataToSave["locale"]; | ||
| 130 | } | 166 | } | ||
jjazeix: "modes" can be renamed to be more precise | |||||
| Context not available. | |||||
| 137 | if(newLocale.indexOf('.') != -1) { | 173 | if(newLocale.indexOf('.') != -1) { | ||
| 138 | newLocale = newLocale.substring(0, newLocale.indexOf('.')) | 174 | newLocale = newLocale.substring(0, newLocale.indexOf('.')) | ||
| 139 | } | 175 | } | ||
| 140 | dataToSave = {"locale": newLocale } | 176 | dataToSave = {"locale": newLocale} | ||
| 177 | items.mode = items.mode === "easyMode" ? "easy" : "normal"; | ||||
| 178 | dataToSave["mode"] = items.mode | ||||
| 179 | | ||||
| 180 | Activity.initLevel() | ||||
| 141 | 181 | | |||
| 142 | background.locale = newLocale; | 182 | background.locale = newLocale; | ||
| 143 | 183 | | |||
qsTr needs to be used for translatable texts. jjazeix: qsTr needs to be used for translatable texts.
Easy does not help the child, we should find a… | |||||
jjazeix: you should use a GCDialogCheckBox instead of directly a GCText | |||||
jjazeix: no need to change the text color | |||||
it should only be changed when clicking on the cross (so in onSaveData). jjazeix: it should only be changed when clicking on the cross (so in onSaveData).
You should also check… | |||||
I still don't understand why you use 2 different variables for the same data jjazeix: I still don't understand why you use 2 different variables for the same data | |||||
| Context not available. | |||||
| 220 | 260 | | |||
jjazeix: you should not change the text directly | |||||
| 221 | GCText { | 261 | GCText { | ||
| 222 | id: questionItem | 262 | id: questionItem | ||
| 223 | anchors.right: planeText.right | 263 | anchors { | ||
| 224 | anchors.rightMargin: 2 * plane.width / 3 | 264 | right: planeText.right | ||
| 225 | anchors.verticalCenter: planeText.verticalCenter | 265 | rightMargin: 2 * plane.width / 3 | ||
| 226 | anchors.bottomMargin: 10 * ApplicationInfo.ratio | 266 | //Turned this off as the letter was going out of the banner | ||
| 267 | //verticalCenter: planeText.verticalCenter | ||||
| 268 | bottomMargin: 10 * ApplicationInfo.ratio | ||||
| 269 | } | ||||
| 227 | fontSize: hugeSize | 270 | fontSize: hugeSize | ||
| 228 | font.weight: Font.DemiBold | 271 | font.weight: Font.DemiBold | ||
| 229 | color: "#2a2a2a" | 272 | color: "#2a2a2a" | ||
| Context not available. | |||||
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).