Code review comment for lp://qastaging/~verzegnassi-stefano/ubuntu-docviewer-app/document-hub2

Revision history for this message
Riccardo Padovani (rpadovani) wrote :

Last 2403 lines of diff (rev 90).
Unfortunately they aren't here on Launchpad. So I locally merged this branch in the trunk, then I used qdiff, I report here interesting pieces:

src/app/qml/pdfView/PdfViewGotoDialog.qml
+ Button {
+ objectName:"GOButton"
+ text: i18n.tr("GO!")
+ color: UbuntuColors.orange

Orange isn't anymore suggested by design guidelines[0]. I suggest to use green or gray

###
src/app/qml/textView/TextView.qml

Why do you use a TextArea with readOnly: true and not a Label?

###
src/app/quick/documentmodel.cpp

Could you please explain what are you trying to achieve with DocumentModel::_q_directoryChanged?
Seems improvable, but I'm not sure I understand how you want to use it

DocumentModel::parseDirectoryContent
item.dateDiff = 0;
Probably is better to add a ENUM for this

Great work Stefano, congrats!

All issues I pointed out are minor, and the improvement is huge :-)

I'm going to test it

[0]https://design.ubuntu.com/apps/building-blocks/buttons

review: Needs Fixing (code)

« Back to merge proposal