add keyboard navigation in simplepaint
Needs ReviewPublic

Authored by jjazeix on Thu, Nov 29, 6:45 PM.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
jjazeix created this revision.Thu, Nov 29, 6:45 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptThu, Nov 29, 6:45 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript

Thank you for the PR, I did some comments

src/activities/simplepaint/PaintCursor.qml
1 ↗(On Diff #46494)

update the filename

src/activities/simplepaint/Simplepaint.qml
44 ↗(On Diff #46494)

the coding rules incites to add a space before the {
In the if condition too (if(true) { ... )

48 ↗(On Diff #46494)

can you add spaces before and after '='?

99 ↗(On Diff #46494)

isColorTab = !isColorTab

146 ↗(On Diff #46494)

use an "int" if you already know the type. var is slower

349 ↗(On Diff #46494)

I think you can directly put the background.refreshCursor() in the initLevel().
In this case, you don't need to call it on onPreviousLevelClicked and onNextLevelClicked as they call the initLevel()

jjazeix added inline comments.Fri, Nov 30, 7:30 AM
src/activities/simplepaint/Simplepaint.qml
163 ↗(On Diff #46494)

can you clean up the log, it's not needed when we release

jjazeix updated this revision to Diff 46558.Fri, Nov 30, 2:12 PM
jjazeix updated this revision to Diff 46560.Fri, Nov 30, 2:15 PM
jjazeix updated this revision to Diff 46586.Fri, Nov 30, 8:45 PM

Good for me. Please push it and I'll update the icon with a new one more appropriate.

jjazeix updated this revision to Diff 47366.Tue, Dec 11, 7:45 PM
jjazeix updated this revision to Diff 47384.Tue, Dec 11, 8:16 PM
jjazeix updated this revision to Diff 47385.