mart added inline comments.

INLINE COMMENTS

> InlineMessagesGallery.qml:61
> +            wrapMode: Text.WordWrap
> +            text: "Inline messages allow you to show various types of 
> messages without an overlay dialog."
> +        }

add some usage explanation here, like that should be added in the main layout 
of the ui you want to put it in, that is not visible by default, needing an 
explicit visible=true

a two rows synopsis of the ig would also be good here tough we have to wait for 
that i guess

> InlineMessage.qml:91
> +            // contentLayout) don't work.
> +            ScriptAction { script: contentItem.opacity = 1.0 }
> +        }

this would make opacity of contentitem 1.0 even when animationg to 0?

> InlineMessage.qml:182
> +            id: icon
> +
> +            width: Kirigami.Units.iconSizes.smallMedium

also a color: root.icon.color (no checks needed)

> InlineMessage.qml:192
> +            source: {
> +                if (root.icon) {
> +                    return root.icon;

with qqc2 api, here would return either root.icon.source or if not set 
root.icon.name

> InlineMessage.qml:276
> +
> +            Repeater {
> +                model: root.actions

i kindof feel we should make a private component out of that, for overflowable 
toolbars..  (or maybe even not private) but this is for the future, this is 
perfect for now

> hein wrote in InlineMessage.qml:387
> I'm not sure it's a problem it will animate when you unhide with a parent.

if you need to know if an item is visible by itself, not caring of parents 
visibility, use opacity instead, at least that's what qt docs say.

> InlineMessage.qml:79
> +     */
> +    signal linkHovered(string link)
> +

what is the use case, tooltips?

> InlineMessage.qml:100
> +     */
> +    property string icon
> +

even if at the beginning not 100% supported, i would prefer it making 
api-compatible with qqc2 (even if i don't like that api that much)
there is a class in controls/private called ActionIconGroup which exports the 
ptoperties that qqc2 uses for icons, so icon.name,icon.source, icon.color
not all of those need to be supported (i prefer to ignore icon.size usially) 
but at least name,source and color should

> InlineMessage.qml:127
> +     */
> +    readonly property bool animating: 
> contentItem.hasOwnProperty("animating") && contentItem.animating
> +}

what is the use case for exposing this property in the public api?

> enums.h:39
>  
> +class InlineMessageType : public QObject
> +{

please call this in a more generic way, like MessageSeverity, so can be used 
for other kinds of messages if needed, like passivenotification and inline 
dialogs

REPOSITORY
  R169 Kirigami

REVISION DETAIL
  https://phabricator.kde.org/D11663

To: hein, #kirigami, mart
Cc: davidedmundson, ngraham, broulik, plasma-devel, apol, mart, hein

Reply via email to