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

Revision history for this message
Olivier Tilloy (osomon) wrote :

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.

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

In the bookmarks view, when the right panel has active focus the current folder in the left panel should still be highlighted.

In the bookmarks view, the font color for the current bookmark in the right panel shouldn’t be orange.

It seems to be that favicons in the view are too large, and that the font size for delegates is too big as well.

In the visual spec the folders list has a left margin, and folders are separated by horizontal lines.

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

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)]

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

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).

Can NewTabLandscapeView be renamed NewTabViewWide?

review: Needs Fixing

« Back to merge proposal