Differential D27994

Improvement in Solar System Hints
ClosedPublic
Actions

Authored by anujyadav on Mar 11 2020, 7:17 PM.

Details

Reviewers
jjazeix
Group Reviewers
GCompris: Improvements
Maniphest Tasks
T12339: solar system: improve hint system
Summary

Ref T12339

  • I have added an extra hint for the position of planets , this rhyme can help students learn positions easily
  • Now hints will be displayed to only particular questions

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
anujyadav created this revision.Mar 11 2020, 7:17 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptMar 11 2020, 7:17 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
anujyadav requested review of this revision.Mar 11 2020, 7:17 PM
timotheegiet added a subscriber: timotheegiet.Mar 11 2020, 7:27 PM
timotheegiet added inline comments.
src/activities/solar_system/SolarSystem.qml
308

a single ! is enough.
Also, please put the string in qsTr to make it translatable.

anujyadav edited the summary of this revision. (Show Details)Mar 11 2020, 7:27 PM
anujyadav changed the visibility from "Public (No Login Required)" to "GCompris: Improvements (Project)".
timotheegiet added inline comments.Mar 11 2020, 7:32 PM
src/activities/solar_system/SolarSystem.qml
290

I'm not sure how easy this will be to localize for different languages.

308

And also maybe capitalization of all the words is not necessary...

anujyadav updated this revision to Diff 77459.Mar 11 2020, 8:43 PM
anujyadav marked 2 inline comments as done.

I have made the changes . Also should I remove the new hint provided

timotheegiet added a comment.Mar 11 2020, 9:38 PM

No need to remove the hint, but please add context for translators.

src/activities/solar_system/SolarSystem.qml
290

After a bit of search I guess it should be possible to find similar equivalent in all languages.
Still it will need to be carefully managed by translators, better add some context before with //: (see Qt documentation about that: https://doc.qt.io/qt-5/qtquick-internationalization.html ), saying something like "Find an equivalent sentence in your language which starts with the first letter of each planet name in the right order".

anujyadav updated this revision to Diff 77507.Mar 12 2020, 2:21 PM

I have added the context to the hint

jjazeix added a subscriber: jjazeix.Mar 14 2020, 3:50 PM

There is an issue with the patch, it shouldn't be displayed like that. We should have a diff of all the files separately.

mypatch.patch
416 ↗(On Diff #77507)

Please, add space around ===, remove the one before ), and add a space between ) and {.

418 ↗(On Diff #77507)

indentation

419 ↗(On Diff #77507)

add space after else

419 ↗(On Diff #77507)

it may be better to not having the hint button displayed when there are no hints available instead of having a hint button saying there is no hint.

420 ↗(On Diff #77507)

use camelCase for naming variables (noHintDialog, hintProvided)

anujyadav added a comment.Mar 19 2020, 6:24 PM
In D27994#627413, @jjazeix wrote:

There is an issue with the patch, it shouldn't be displayed like that. We should have a diff of all the files separately.

Can you please help me on how to make changes in the bar with each changing question ? Also sorry for the late reply

jjazeix added a comment.Mar 19 2020, 7:16 PM
In D27994#630719, @anujyadav wrote:

Can you please help me on how to make changes in the bar with each changing question ? Also sorry for the late reply

Which issue do you face?

anujyadav added a comment.Mar 20 2020, 9:08 AM
In D27994#630759, @jjazeix wrote:
In D27994#630719, @anujyadav wrote:

Can you please help me on how to make changes in the bar with each changing question ? Also sorry for the late reply

Which issue do you face?

I am not getting how to make change in bar as you change a question , I think because content of bar depends on the planet you choose

jjazeix added a comment.Mar 21 2020, 8:55 PM
In D27994#631021, @anujyadav wrote:
In D27994#630759, @jjazeix wrote:
In D27994#630719, @anujyadav wrote:

Can you please help me on how to make changes in the bar with each changing question ? Also sorry for the late reply

Which issue do you face?

I am not getting how to make change in bar as you change a question , I think because content of bar depends on the planet you choose

Instead of having a js variable Activity.hintprovide, you can have it in qml object "items".
There are already several BarEnumContent with or without hints, you can use the good one depending on the value of hintprovide

anujyadav updated this revision to Diff 78193.Mar 21 2020, 9:48 PM

I think now its working properly , hint button is displaying only for particular questions

jjazeix added a comment.Mar 22 2020, 9:12 AM

the diff seems incomplete, it misses your previous changes

anujyadav updated this revision to Diff 78223.Mar 22 2020, 1:10 PM

updated diff with all changes made

anujyadav changed the visibility from "GCompris: Improvements (Project)" to "Public (No Login Required)".Mar 23 2020, 9:17 PM
jjazeix added inline comments.Mar 24 2020, 7:21 PM
src/activities/solar_system/SolarSystem.qml
83

hintProvided instead of hintprovide everywhere please

jjazeix added inline comments.Mar 24 2020, 7:23 PM
src/activities/solar_system/Dataset.js
32

it is better to use true or false for boolean values

anujyadav updated this revision to Diff 78452.Mar 25 2020, 2:08 PM

changes made and context also added

jjazeix accepted this revision.Mar 27 2020, 9:45 AM

Hi, under which name should I commit? "FirstName LastName <email>" for the syntax :)

src/activities/solar_system/SolarSystem.qml
394

it should not be needed as we should not display the hint icon if no hint is provided (meaning we can't click on it)

This revision is now accepted and ready to land.Mar 27 2020, 9:45 AM
anujyadav added a comment.Mar 27 2020, 7:00 PM
In D27994#635649, @jjazeix wrote:

Hi, under which name should I commit? "FirstName LastName <email>" for the syntax :)

Anuj Yadav <yadavanuj952@gmail.com>

jjazeix closed this revision.Mar 27 2020, 7:41 PM

thanks, commited in b0592dbc7f54bcaba0cb1cde3f36beb2185b1555

src/activities/solar_system/SolarSystem.qml
394

I'll do the change when commiting, don't bother changing it

Revision Contents
Changeset List

Diff 78452

View Options

src/activities/solar_system/Dataset.js

Loading...
View Options

src/activities/solar_system/SolarSystem.qml

Loading...
View Options

src/activities/solar_system/solar_system.js

Loading...