D28383: Add PageRouter component

Marco Martin noreply at phabricator.kde.org
Wed Apr 15 10:38:53 BST 2020


mart added inline comments.

INLINE COMMENTS

> tst_pagerouter.qml:56
> +        id: router
> +        initialRoute: "/home"
> +        columnView: root.columnView

I find all the "/" in the examples confusing, not sure is a good naming convention (and works prefectly without having that /, and i don't necessarly care what the convention in other frameworks is).
in the hend you can't push directly something like "/home/login" as a single string so doesn't really make sense as paths, i would prefer examples had simple strings as route names, so your route would be indicated as ["home", "login"]

> pagerouter.cpp:214
> +
> +void PageRouter::navigateToRoute(QJSValue route)
> +{

I wouldn't expect a verb like navigate to be always destructive.
if the route happens to be the same, then the function should be a no-op
if the current route is
home/foo/bar and i want to navigate to home/foo/baz i would expect to remove and destroy only bar, keeping the first two pages

if the new route is foo/bar/baz, should just push the new one keeping all is there

> pagerouter.h:180
> +     */
> +    Q_PROPERTY(ColumnView* columnView MEMBER m_columnView NOTIFY columnViewChanged)
> +

I know we are fixed to columnview now and its fine, but i would prefer the property being called pageStack like in applicationwindow (in reality i would like both being called pageview but it's too late) which doesn't assume which it is, if sme day we would like to support stackview or whatever else we wouldn't have a sore point in the api

also, to the property assigning the pagerow (which will look for the columnview internally, but not exposed trough the api)

> pagerouter.h:357
> +QML_DECLARE_TYPEINFO(PageRouter, QML_HAS_ATTACHED_PROPERTIES)
> \ No newline at end of file


newlines!

REPOSITORY
  R169 Kirigami

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

To: cblack, #kirigami, mart, davidedmundson
Cc: ahiemstra, davidedmundson, plasma-devel, fbampaloukas, GB_2, domson, dkardarakos, ngraham, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200415/8ca9e6c8/attachment-0001.html>


More information about the Plasma-devel mailing list