Code review comment for lp:~unity-2d-team/unity-2d/shortcut-hint-overlay

> Looks nice! Aside from above comments:
> - along with blurred background, the sheen is also needed. See Background.qml
> - background has unusual colour tint set. We don't have tinting just now, so
> should be removed.
I checked with design, this is the conversation:
dyams rosie: should we need to have a background for overlay hint?
dyams rosie: if the overlay is displayed on a white background for ex: gedit window, the shortcuts are hardly visible
rosie dyams: there is a 35% #5e2750 layer and a -80% (-150% - 150%) brightness layer

> - shell/Shell.qml - why remove "KeyNavigation.left: launcherLoader" ?
The only difference i see in Shell.qml is
30 + Loader {
31 + id: shortcutHintLoader
32 + anchors.centerIn: parent
33 + }
34 +

> - in symmetricKey() and getShortcutKey(), GConfItem declared in Unity2D, no
> need to import QtQuick
Are you sure? FYI, i get this '[WARNING] ShortcutHint.qml:22:1: Item is not a type'
> - in your javascript, your notation is mixing camelCase and
> underscore_separator. Use camelCase please. Some spacing between arguments in
> the function calls would help readability too.
> - symmetricKey deserves a comment to explain why it is there.
Done.
> - can you check with design about using empty strings if no key is set? I
> think it looks confusing.
true, but its design decision.

> - I have a truncation for a long key combination: https://imgur.com/a/okLFf
> can you also check with design to see what to do?
Design changed a bit, I have updated the same.

> - overlay needs to be positioned a little higher, see comparison image:
> https://imgur.com/xqAVf
It is positioned to the center. I doubt we need to adjust it again.

> - there are typos, and some EnglishUK spellings instead of US (see first
> screenshot again). Yes they're in the mockup, but check with design
Done

> - in "Switching" - the left/right cursor keys do not move focus in Metacity.
> Is this something that needs to be enabled, or should this listing be removed?
No, It works

> - Alt+Middle Mouse drag also doesn't work for me.
No, It works

> - Please use fontUtils.js to specify font sizes
Done.

> - you have unrelated changes from tests/run-tests.rb &
> tests/shell/input_shaping.rb in the diff, please remove.
Is it? but they are not listed under the 'differences' here.

> - recently we decided having authors names in text files was going to be a
> pain, so we removed them. Will you do the same?
Done.

> - have you given any thought to accessibility? How will this work with Orca?
> Since focus is not stolen, what will Orca do? Will it be possible to have Orca
> read out these shortcuts in some way? Just something to think about.
No.

> - import Unity2d 1.0 /* required by GConfItem */ <- GConfItem requires
> Unity2D, not other way around
Yes I know.

> - qml (I know, Tiago's) could be cleaned up a little, with both x,y
> coordinates & anchors being used, lots of unnecessary margin:0 scattered about
> for example.
Done.