keyboard binding support to "Money" based activities in gcompis
ClosedPublic

Authored by smitpatil on Thu, Dec 6, 7:57 AM.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
smitpatil created this revision.Thu, Dec 6, 7:57 AM
Restricted Application added a project: KDE Edu. · View Herald TranscriptThu, Dec 6, 7:57 AM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
smitpatil requested review of this revision.Thu, Dec 6, 7:57 AM
amankumargupta added inline comments.
src/activities/money/MoneyCore.qml
135

Needs spaces (parent.height * 1.1). Same for above.

140

Needs space (z: -1)

300

width and height as multiples of 1.1 should be good, as you had done above. Also space needed.

305

Space needed. Refer above.

316

Empty line not needed here.

318

Empty line not needed here.

319

Space needed (items.area.count !== 0)

320

Space needed (items.area.itemAt(items.itemIndex).selected = false)

324

Space needed (items.area = answer)

328

Space needed (items.area = pocket)

330

Space needed.

333

Space needed, and remove extra space, if(items.area.count !==0) {

334

Empty line not needed.

335

Space needed. Same for the statement below.

338

Since you were already using if-else statements above, can you maintain the consistency by replacing the switch case statements with if-else?

339

Extra blank line not needed.

341

Space needed on left of != operator.

347

Space needed.

350

Space needed after = operator.

358

Space needed

361

Empty line not needed.

364

Space needed on sides of = operator

366

Empty line not needed.

369

One extra empty line not needed.

src/activities/money/money.js
716

Space needed on left of = operator.

720

One extra empty line not needed.

amankumargupta requested changes to this revision.Thu, Dec 6, 8:20 AM
This revision now requires changes to proceed.Thu, Dec 6, 8:20 AM
smitpatil updated this revision to Diff 46942.Thu, Dec 6, 9:03 AM
smitpatil updated this revision to Diff 46943.Thu, Dec 6, 9:08 AM
smitpatil marked 14 inline comments as done.
smitpatil marked 11 inline comments as done.
smitpatil added inline comments.Thu, Dec 6, 9:13 AM
src/activities/money/MoneyCore.qml
338

I first tried if-else but that took a lot of lines. Thus, I find switch more consistent than if-else. still, if you want, I will implement it.

amankumargupta added inline comments.Thu, Dec 6, 9:30 AM
src/activities/money/MoneyCore.qml
137

spaces before and after : symbol.

298

space before {

338

Well, as far as I can see, the case statements will just be replaced with the if conditional statements. So, the number of lines should technically be around same :) You can once implement it and check ;)

smitpatil updated this revision to Diff 46945.Thu, Dec 6, 10:19 AM
smitpatil marked 4 inline comments as done.
smitpatil updated this revision to Diff 46948.Thu, Dec 6, 10:33 AM
amankumargupta accepted this revision.Thu, Dec 6, 10:40 AM
This revision is now accepted and ready to land.Thu, Dec 6, 10:40 AM
smitpatil updated this revision to Diff 46986.Thu, Dec 6, 6:12 PM
jjazeix accepted this revision.Thu, Dec 6, 6:17 PM