Merge lp://qastaging/~fabiozaramella/foto/use-bottom-toolbar into lp://qastaging/foto/foto-1.0

Proposed by Fabio Zaramella
Status: Merged
Approved by: Erasmo Marín
Approved revision: 111
Merged at revision: 105
Proposed branch: lp://qastaging/~fabiozaramella/foto/use-bottom-toolbar
Merge into: lp://qastaging/foto/foto-1.0
Diff against target: 964 lines (+209/-367)
13 files modified
CMakeLists.txt (+1/-2)
po/foto.pot (+1/-1)
src/AppWindow.vala (+1/-0)
src/dialogs/AddToAlbumDialog.vala (+12/-7)
src/pages/AlbumPage.vala (+4/-37)
src/pages/CollectionPage.vala (+81/-45)
src/pages/LastImportedPage.vala (+2/-36)
src/pages/LibraryPage.vala (+6/-39)
src/pages/Page.vala (+9/-55)
src/pages/TagPage.vala (+4/-37)
src/widgets/ItemSearchBar.vala (+0/-48)
src/widgets/ItemSortBar.vala (+0/-60)
src/widgets/ItemSortSearchBar.vala (+88/-0)
To merge this branch: bzr merge lp://qastaging/~fabiozaramella/foto/use-bottom-toolbar
Reviewer Review Type Date Requested Status
Fabio Zaramella Needs Information
Erasmo Marín Needs Fixing
Review via email: mp+236733@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2014-09-26.

To post a comment you must log in.
Revision history for this message
Erasmo Marín (erasmo-marin) wrote : Posted in a previous version of this proposal

really nice work, some questions:
-Can be the border of the toolbar only applied in the top border?
-why not to put the selection button in the bottom toolbar? import to left, and search to right?

a suggestion:
-The "unselect" button looks useless to me, because when you are in single mode selection, clicking the selected item deselect it, and when you are in multiple selection, you just click the selection button and all items are unselected automatically. It can be removed.

review: Needs Information
Revision history for this message
Fabio Zaramella (fabiozaramella) wrote : Posted in a previous version of this proposal

I think that the normal location for the import button is on the right, such as in noise or vocal and many other apps, so search has to be on the left to not make the toolbar crowded.

However i removed the unselect button, but i didn't understand whats wrong with the border/box-shadow of the toolbar, it looks pretty good to me. :)

Revision history for this message
Fabio Zaramella (fabiozaramella) wrote : Posted in a previous version of this proposal

Oh and i think that the multiple selection button has to be on the top toolbar because it is an action that applied to the whole album, such as search and sort and it will also look a bit out of place in the bottom toolbar being sensitive by default when other buttons aren't.

Revision history for this message
Erasmo Marín (erasmo-marin) wrote : Posted in a previous version of this proposal

What I mean is this:

http://i.imgur.com/OB5Ykgo.png

it looks ugly to me

Revision history for this message
Erasmo Marín (erasmo-marin) wrote : Posted in a previous version of this proposal

also, the search button should be always at right, because of usability reasons. Search options are always at right. My idea was to put the search entry at right of filter options. That way would have only search button, and not search and filter. It's reasonable, because if the user uses a filter, he does it because he is searching.

The import options is a problem, I really don't know what to do with it.

Revision history for this message
Fabio Zaramella (fabiozaramella) wrote : Posted in a previous version of this proposal

i'm ok about merging the sort and search toolbars but i think that the search button then should remain on the left of the toolbar, having only the multiple selection button looks.. mmh i don't know, poor?

Revision history for this message
Erasmo Marín (erasmo-marin) wrote :

Hi Fabio, I want to merge this branch, but it still has some problems compiling. Maybe you have uncommitted changes? thanks!

review: Needs Fixing
111. By ffabio-96-x

try to fix merge problems

Revision history for this message
Fabio Zaramella (fabiozaramella) wrote :

can you tell me if is it ok now? Otherwise can you paste the error please?

review: Needs Information

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: