Code review comment for lp:~artmello/webbrowser-app/webbrowser-app-bookmarks_view

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

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?

review: Needs Fixing

« Back to merge proposal