Code review comment for lp:~uriboni/webbrowser-app/new-tab-wide-format

> When scrolling the "top sites" view, the first item is drawn on top of the
> sections header. Either the header should have a higher z-order than the list
> view, or the list view should be clipped.

Done

> The background color of the sections header doesn’t match the visual spec (htt
> ps://docs.google.com/presentation/d/1ggKmkxUFR5xCBcvkjJ4On9b4iKzEqcycz4hjc16tB
> Go/edit#slide=id.g2bddb9a1a_038).

This and all the other visuals-related comments you made refer to the above linked document, which is the UX spec, not the visual spec. The current design follows the visual spec as closely as possible (minus some changes that will come from the UITK): https://docs.google.com/presentation/d/1woHjO8K4iqyVZZlfQ4BXL0DhYbwkEmZ7wvcUhYzHDRk/edit?pli=1#slide=id.gb81843c7e_0_42

> According to the specification: « User can drag a bookmark in a folder.
> Bookmarks can only exist in one folder at a time ».

I consulted with design and they need to provide some extra UX and visual guidance. They will do so next week but in the meantime I am adding a provisional implementation based on advice I got from James over IRC.

> On my vivid desktop, with the UITK staging branch,
> QmlTests::NewTabLandscapeView::test_switch_sections_by_keyboard() is reliably
> failing:
> 2: FAIL! :
> QmlTests::NewTabLandscapeView::test_switch_sections_by_keyboard() Compared
> values are not the same
> 2: Actual (): 0
> 2: Expected (): 1
> 2: Loc: [/home/osomon/dev/phablet/browser/webbrowser-
> app/tests/unittests/qml/tst_NewTabLandscapeView.qml(191)]

This is caused by a recent change in the UITK staging branch. I submitted a bug to track it: https://bugs.launchpad.net/ubuntu/+source/ubuntu-ui-toolkit/+bug/1481233

> Although not specified, there should probably be scrollbars for all the
> listviews.

Done

> Can you check with design whether we really want all the top sites section to
> not have a limit? I’d personally add a limit (maybe 10, or 20 top sites max).

James suggested trying with a limit of 10. This is implemented now.

> Can NewTabLandscapeView be renamed NewTabViewWide?

Done