[Merge] lp:~artmello/webbrowser-app/webbrowser-app-bookmarks_view into lp:webbrowser-app

Olivier Tilloy olivier.tilloy at canonical.com
Wed Oct 7 15:03:44 UTC 2015


Review: Needs Fixing

In BookmarksFoldersView.qml, is the onActiveFocusChanged handler really needed? Given that the top-level item is a focus scope, simply setting 'focus: true' on bookmarksFolderListView should achieve the same result I think.

The same applies to BookmarksView.qml and BookmarksViewWide.qml, which should be made FocusScopes.

In BookmarksFoldersViewWide.qml, in restoreLastFocusedColumn() there is one extraneous blank line, please remove it.

The bookmarkEntryRemoved signal, in both versions of the bookmarks view, doesn’t seem to be useful, as the view already has a reference to the bookmarks model, so it could do the removal itself, and then call done().

The headers in bookmarks and history views shouldn’t be plain white. See the visual spec at https://docs.google.com/presentation/d/1P6A7ZsI03sPfuI9vzPeC0VcUfIgugxnU4QyObH6UNTs/edit#slide=id.gde1819d92_0_109.

In wide mode, the "all bookmarks" section always contains the homepage, as a hardcoded entry. In narrow mode, it doesn’t. Can we make the two views consistent, by displaying the homepage entry on both?
-- 
https://code.launchpad.net/~artmello/webbrowser-app/webbrowser-app-bookmarks_view/+merge/270613
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.



More information about the Ubuntu-reviews mailing list