> On Sept. 5, 2013, 10:47 a.m., Aaron J. Seigo wrote: > > File Attachment: ratings-main.png > > <http://git.reviewboard.kde.org/r/112533/#fcomment97> > > > > I wonder if we need an explicit "Ratings" header here. Replacing the > > raw numbers with stars (or some other icon, but stars are well recognized) > > should make it obvious that these are ratings. So it probably doesn't need > > to say that explicitly. > > > > A header here adds more visual noise and takes up screen space.
the Ratings header is gone > On Sept. 5, 2013, 10:47 a.m., Aaron J. Seigo wrote: > > File Attachment: ratings-main.png > > <http://git.reviewboard.kde.org/r/112533/#fcomment98> > > > > instead of numbers, using stars would be better. > > > > the labels should also be center aligned: > > > > Usability: ** > > Funny: *** > > I couldn't find a star icon. Can you suggest one? Also my labels aren't aligned in the center. I have attached a new Image. What am I doing wrong? > On Sept. 5, 2013, 10:47 a.m., Aaron J. Seigo wrote: > > activeclient/package/contents/ui/storebrowser/AssetColumn.qml, lines 51-53 > > <http://git.reviewboard.kde.org/r/112533/diff/1/?file=187171#file187171line51> > > > > this seems very brute force; just re-setting the assetId was not enough? > > > > i'm also a little concerned about popping the column from *inside* the > > column itself. i'm sure QML does the right thing behind the scenes > > w/regards to memory allocation, etc but it would be far nicer if simply > > re-setting the assetId would do the trick, or even if page offered a reload > > function that re-sent the asset info request. I have moved the functionality of onAssetIdChanged to reloadPage and it works. > On Sept. 5, 2013, 10:47 a.m., Aaron J. Seigo wrote: > > activeclient/package/contents/ui/storebrowser/AssetColumn.qml, lines 299-305 > > <http://git.reviewboard.kde.org/r/112533/diff/1/?file=187171#file187171line299> > > > > there was something about this button that just never felt right. it > > was Yet Another Button in an already pretty busy area of the UI. > > > > when looking at the Ratings page in the settings, it came to me: the > > Delete button should be there. each rating in the Participant Ratings list > > could have a delete button. that way a function which will be rarely used > > is not in the main application UI and is instead in the configuration / > > settings area where it fits in quite nicely. Each rating is a new row in our model. So if we have the asset 3 with two ratings we will have two rows, so when I delete one of the ratings both of them will get deleted because that's how our API works. It deletes all the ratings of an asset. I have also attached an image which shows the new UI. Is that ok? Should we modify something else from the ui? - Giorgos ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112533/#review39403 ----------------------------------------------------------- On Sept. 5, 2013, 10:25 a.m., Giorgos Tsiapaliokas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/112533/ > ----------------------------------------------------------- > > (Updated Sept. 5, 2013, 10:25 a.m.) > > > Review request for Bodega. > > > Description > ------- > > This patch implements the ratings feature > > > Diffs > ----- > > activeclient/package/contents/ui/SettingsPage.qml fb46eca > activeclient/package/contents/ui/settings/ParticipantRatings.qml > PRE-CREATION > activeclient/package/contents/ui/storebrowser/AssetColumn.qml a678f16 > activeclient/package/contents/ui/storebrowser/Ratings.js PRE-CREATION > activeclient/package/contents/ui/storebrowser/RatingsColumn.qml > PRE-CREATION > activeclient/src/bodegastore.h 1e5aac5 > activeclient/src/bodegastore.cpp ba9dc27 > lib/bodega/CMakeLists.txt 8d382a7 > lib/bodega/assetjob.h 5aab88c > lib/bodega/assetjob.cpp 5f539cb > lib/bodega/assetoperations.h 7ce7900 > lib/bodega/assetoperations.cpp 9f9c2d5 > lib/bodega/assetratingsjob.h PRE-CREATION > lib/bodega/assetratingsjob.cpp PRE-CREATION > lib/bodega/assetratingsjobmodel.h PRE-CREATION > lib/bodega/assetratingsjobmodel.cpp PRE-CREATION > lib/bodega/globals.h 5ab45da > lib/bodega/participantratingsjob.h PRE-CREATION > lib/bodega/participantratingsjob.cpp PRE-CREATION > lib/bodega/participantratingsjobmodel.h PRE-CREATION > lib/bodega/participantratingsjobmodel.cpp PRE-CREATION > lib/bodega/ratingattributesjob.h PRE-CREATION > lib/bodega/ratingattributesjob.cpp PRE-CREATION > lib/bodega/ratingsmodel_p.h PRE-CREATION > lib/bodega/ratingsmodel_p.cpp PRE-CREATION > lib/bodega/session.h d27d284 > lib/bodega/session.cpp a7c7e94 > lib/bodega/session_p.h ebefb4f > > Diff: http://git.reviewboard.kde.org/r/112533/diff/ > > > Testing > ------- > > check the attached images > > > File Attachments > ---------------- > > > > http://git.reviewboard.kde.org/media/uploaded/files/2013/09/05/ratings-main.png > > > http://git.reviewboard.kde.org/media/uploaded/files/2013/09/05/participantratings.png > > > Thanks, > > Giorgos Tsiapaliokas > >
_______________________________________________ Active mailing list Active@kde.org https://mail.kde.org/mailman/listinfo/active
