hein marked 5 inline comments as done. hein added inline comments. INLINE COMMENTS
> broulik wrote in InlineMessagesGallery.qml:45 > Wrong class name Thanks, will fix. > broulik wrote in InlineMessagesGallery.qml:115 > There's no `start-here` in Breeze, just `start-here-kde` which won't fall > back to `start-here` (which is distro-branded for you apparently) I need something generic that's not tied to Breeze, hmm ... suggestions? > broulik wrote in InlineMessage.qml:364 > `findIndex` is new in Qt 5.9 > > [1] https://doc.qt.io/qt-5/qtqml-javascript-functionlist.html This was copy-pasted from the Cards code (that's why Marco's copyright is in the header). Marco? > broulik wrote in InlineMessage.qml:387 > Note that `visible` propagates recursively so when the component containing a > default-visible message widget isn't shown this will still animate. But then > I don't know if that's a thing and how to fix that.. I'm not sure it's a problem it will animate when you unhide with a parent. > broulik wrote in InlineMessage.qml:100 > Should this be a `var` property so you could also pass a `QIcon` or "QtQuick > Controls Icon"? I wouldn't have any objections, but Kirigami.Icon.source is 'string' too, so it wouldn't work currently. > broulik wrote in InlineMessage.qml:113 > `KMessageWidget` names it `closeButtonVisible` Yes, but as mentioned in the review description I named it showCloseButton for consistency with another Kirigami component. > broulik wrote in InlineMessage.qml:127 > `contentItem.hasOwnProperty("animating")` Will do. > broulik wrote in enums.h:39 > With Qt 5.8 this could become > > namespace InlineMessageType > { > Q_NAMESPACE > enum Type { > ... > > with `qmlRegisterUncreatableMetaObject` > (purely informational comment) Could be a nice follow-up, but it's good practice to emulate surrounding style in a patch. > broulik wrote in enums.h:42 > `Q_ENUM` > > (also informational, the code around it does the same, could be cleaned up > eventually) See above. REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D11663 To: hein, #kirigami, mart Cc: broulik, ngraham, plasma-devel, apol, davidedmundson, mart, hein
