Code review comment for lp:~mzanetti/unity8/right-edge-2

Revision history for this message
Michael Zanetti (mzanetti) wrote :

> +++ qml/Stages/TransformedSpreadDelegate.qml
>
> Since this relies heavily on the animation "stages" reported by the
> spreadView, a comment explaining the different behaviours of the tile in each
> stage (mentioning the position markers) would help a lot.

done

>
> + property real animatedProgress: 0
> purpose not obvious to me how it's different to "progress", add comment please

done.

>
> + property real tile1StartScale: startScale + .4
> + property real tile0SnapAngle: 10
> could be made readonly, or private actually.

actually intentional in public api as I hope to be able to reuse the TransformedAppDelegate for the tablet which has a less 3D look and feel because of the bigger screen space.

>
> + priv.stage2startTranslate = priv.easingAnimation(0,
> spreadView.positionMarker4, 0, -spreadView.width, spreadView.positionMarker4)
> + spreadView.width;
> confuses me, why the -spreadView.width offset for the animation?

tiles are attached to the right edge (outside) in the beginning and travel a distance of -spreadView.width during the progress of 0..1. So -spreadView.width is the end position. Should be more clear now that I added tons of comments all over the place.

>
> + // As they are static values, lets caluclate
> typo

fixed.

>
> + property real xTranslate: {
> in many of the calculations in this block, there's a "-spreadView.width" being
> used in the calculation of the start or end value in a linearAnimation. Could
> you add a note justifying it?

as explained above.

>
> + return linearAnimation(selectedProgress, negativeProgress,
> selectedXTranslate, selectedXTranslate - spreadView.tileDistance,
> root.progress);
> please wrap

done

>
> + } else if(spreadView.stage == 1) {
> space needed after "if"

done

« Back to merge proposal