Components for Cards
ClosedPublic

Authored by mart on Mar 14 2018, 8:29 AM.

Details

Summary

Cards are divided in AbstractCard which is empty and Card,
which has a default layout of
header: image and title
contentItem: free for any kind of item, but just a Label
with WordWrap recommended
footer: list of action buttons

Test Plan

tested in Kirigami Gallery

Diff Detail

Repository
R169 Kirigami
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mart created this revision.Mar 14 2018, 8:29 AM
Restricted Application added a project: Kirigami. · View Herald TranscriptMar 14 2018, 8:29 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mart requested review of this revision.Mar 14 2018, 8:29 AM
mart changed the visibility from "Public (No Login Required)" to "No One".Mar 14 2018, 8:30 AM
mart edited the test plan for this revision. (Show Details)
mart changed the visibility from "No One" to "Public (No Login Required)".Mar 14 2018, 10:47 AM

Marked some typos. Note that I am not a native speaker, so please re-check them.

Besides that, looks good to me.

examples/gallerydata/contents/ui/gallery/CardsGridViewGallery.qml
61 ↗(On Diff #29469)

If I'm not wrong: allowes -> allows

examples/gallerydata/contents/ui/gallery/CardsLayoutGallery.qml
78 ↗(On Diff #29469)

Its just a -> It's just a
. in -> . In

87 ↗(On Diff #29469)

recomended -> recommended

215 ↗(On Diff #29469)

. if -> . If

247 ↗(On Diff #29469)

.\n in -> .\n In

examples/gallerydata/contents/ui/gallery/CardsListViewGallery.qml
61 ↗(On Diff #29469)

so It is -> so it is

mart updated this revision to Diff 29486.Mar 14 2018, 11:06 AM
  • fix typos
mart marked 6 inline comments as done.Mar 14 2018, 11:07 AM
davidedmundson added inline comments.
src/controls/templates/AbstractCard.qml
69 ↗(On Diff #29486)

are you basing the implicit size from the size of the parent layout?

mart added inline comments.Mar 14 2018, 11:18 AM
src/controls/templates/AbstractCard.qml
69 ↗(On Diff #29486)

no, when it's in a layout the size hints are based purely on the content.

the different behavior is when it's in a gridview or a listview, where its sized is based respectively on the cell size or to be expanding to be as wide as the listview.
it is kindof hacky, but is the only way i found that "just works"

There seems to be a problem with the overflow menu:

mart added inline comments.Mar 14 2018, 11:37 AM
src/controls/templates/AbstractCard.qml
69 ↗(On Diff #29486)

in the latest version i get zero binding loops in fact

davidedmundson requested changes to this revision.Mar 14 2018, 11:54 AM
davidedmundson added inline comments.
src/controls/templates/AbstractCard.qml
69 ↗(On Diff #29486)

The following should behave the same and it keeps all the semantic meaning and it means data flows in the right directions

implicitHeight: mainLayout.implicitHeight + topPadding + bottomPadding
height: internal.completed && internal.gridView
        ? internal.gridView.cellHeight - Kirigami.Units.largeSpacing*2 : implicitHeight

(not tested as I have an error running the gallery, see IRC)

This revision now requires changes to proceed.Mar 14 2018, 11:54 AM
mart updated this revision to Diff 29502.Mar 14 2018, 1:51 PM
  • fix last binging loop
  • update the qrc
  • up to date qmltypes
  • take layout spacing into account
mart added a comment.Mar 14 2018, 1:52 PM

There seems to be a problem with the overflow menu:

fixed in latest version

mart added inline comments.Mar 14 2018, 1:54 PM
src/controls/templates/AbstractCard.qml
69 ↗(On Diff #29486)

seems to behave the same, but with it i have a binding loop on implicitHeight now

apol added a subscriber: apol.Mar 14 2018, 2:45 PM

Worked for me. Had to expose the components in the qmldir:

diff --git a/src/qmldir b/src/qmldir
index f262452..2275600 100644
--- a/src/qmldir
+++ b/src/qmldir
@@ -36,4 +36,6 @@ AbstractApplicationHeader 2.0 AbstractApplicationHeader.qml
 ContextDrawer 2.0 ContextDrawer.qml
 ApplicationWindow 2.0 ApplicationWindow.qml
 PageRow 2.0 PageRow.qml
+AbstractCard 2.4 AbstractCard.qml
+Card 2.4 Card.qml

Also had to rebase to master.

mart added a comment.Mar 14 2018, 5:29 PM
In D11316#225668, @apol wrote:

Worked for me. Had to expose the components in the qmldir:

diff --git a/src/qmldir b/src/qmldir
index f262452..2275600 100644
--- a/src/qmldir
+++ b/src/qmldir
@@ -36,4 +36,6 @@ AbstractApplicationHeader 2.0 AbstractApplicationHeader.qml
 ContextDrawer 2.0 ContextDrawer.qml
 ApplicationWindow 2.0 ApplicationWindow.qml
 PageRow 2.0 PageRow.qml
+AbstractCard 2.4 AbstractCard.qml
+Card 2.4 Card.qml

Also had to rebase to master.

hmm, that qmldir file should be used only for purposes of doxygen indexing, shouldn't affect the components being available?

mart updated this revision to Diff 29514.Mar 14 2018, 5:31 PM
  • add to qmldir
  • Merge branch 'master' into mart/Card
  • add more items to qmldir
mart added a comment.Mar 14 2018, 5:32 PM

now it's on top of master and qmldir has all the public files

examples/gallerydata/contents/ui/gallery/CardsLayoutGallery.qml
83 ↗(On Diff #29514)

Why is it the responsiblility of the user of a CardLayout to set Layout.fillWidth: true on every item yet CardLayout goes to great lengths to auto-inject Layout.fillHeight on every item? Surely one of the other?

(also it seems to visually completely explode if you don't have this set, which sounds like the sign of a bug elsewhere)

src/controls/CardsLayout.qml
35 ↗(On Diff #29514)

I can see you want it in a layout because you're setting the relevant attached properties, but why explicitly Column?

55 ↗(On Diff #29514)

shouldn't this include the spacing? Otherwise it's always ever so slightly smaller than my maxSize

src/controls/private/BannerImage.qml
106 ↗(On Diff #29514)

this will never elide properly, the layout it's filling is not constrained.

   left: root.titleAlignment & Qt.AlignLeft ? parent.left : undefined
right: root.titleAlignment & Qt.AlignRight ? parent.right : undefined

will only have one.

src/controls/templates/AbstractApplicationHeader.qml
88 ↗(On Diff #29514)

What are these changes in the header about?

mart updated this revision to Diff 29585.Mar 15 2018, 11:18 AM
mart marked 3 inline comments as done.
  • fix elide for bannerimage
src/controls/CardsLayout.qml
35 ↗(On Diff #29514)

It's not mandatory but the one i think it makes more sense, so i want to put it as a recommendation.
kirigami pages always scroll only vertically, so makes sense to recommend a vertical development of the final page layout.

src/controls/templates/AbstractApplicationHeader.qml
88 ↗(On Diff #29514)

they were done to put the correct vertical margin on top, to have the same amount of space left/top/right in the card, otherwise there was a too big space in the top margin (which was a generic problem, but with cards it became very apparent)

mart updated this revision to Diff 29626.Mar 15 2018, 6:53 PM
  • better top padding
davidedmundson accepted this revision.Mar 19 2018, 11:52 AM

+1
Probably worth porting some real world code before commiting the new API.

Please commit with split the header changes, and Units change they seem very stand-alone. Can be done when merging if it's a pain to do here.

There's a few thing I don't like, but don't have anything better for, it's very frustrating because QQuickGridView has a virtual method when we add an item which would be the perfect layer to meddle with the delegate item's size.

This revision is now accepted and ready to land.Mar 19 2018, 11:52 AM
mart updated this revision to Diff 29904.Mar 19 2018, 11:58 AM
  • use kirigami namespace
This revision was automatically updated to reflect the committed changes.